From 13371337d20e8f6fd9a2a6548fd4928c788b4b17 Mon Sep 17 00:00:00 2001 From: Billy Lau Date: Tue, 29 Sep 2026 09:01:10 -0500 Subject: [PATCH] automate_observation: warn about ignored flag combinations Some flags are silently ignored depending on what they are combined with. Log a warning at startup for each: - --check_preinstalled_only, --verifier_path, --no_prefetch, --cache_dir, --cache_prefetch_concurrency and --cache_prefetch_timeout without --perform_inclusion_proof_check. - --cache_prefetch_concurrency and --cache_prefetch_timeout with --no_prefetch, since only pre-fetching reads them. - --pull-all-apks with --pull-preinstalled-apks-only, where only pre-installed APKs are pulled. Integer flags count only when set to a non-default value. These are warnings, not errors, so existing invocations keep working and exit codes are unchanged; the only difference is the new log lines for these combinations. The checks live in a pure function, validate_argument_combinations(), so other tools can reuse the rules. Test: - pytest uraniborg/scripts/python/tests/: 150 passed (21 new). - Live on an Android 14 emulator with four ignored inclusion-proof flags: four warnings at startup, run otherwise unchanged (exit 0). - Real process with --perform_inclusion_proof_check --no_prefetch and both tuning flags: both warnings logged, --cache_dir not flagged. Change-Id: I49800dc522b5d1c4939749fe0d3812482adb6b6c --- uraniborg/docs/automate_observation.md | 18 +++ .../scripts/python/automate_observation.py | 50 +++++++ .../python/tests/test_automate_observation.py | 124 ++++++++++++++++++ 3 files changed, 192 insertions(+) diff --git a/uraniborg/docs/automate_observation.md b/uraniborg/docs/automate_observation.md index 1f19f66..fcd98ae 100644 --- a/uraniborg/docs/automate_observation.md +++ b/uraniborg/docs/automate_observation.md @@ -42,6 +42,24 @@ A requested serial that is not connected is not silently skipped: it is logged as an error, reported as `FAILED` in the final summary, and makes the script exit with code `1`. The remaining requested devices are still observed. +### Ignored Flag Combinations + +Some flags only matter together with others. When a flag would be ignored, the +script logs a warning at startup and then runs as it otherwise would; the exit +code is not affected: + +- `--check_preinstalled_only`, `--verifier_path`, `--no_prefetch`, + `--cache_dir`, `--cache_prefetch_concurrency` and `--cache_prefetch_timeout` + have no effect without `--perform_inclusion_proof_check`. The two integer + flags only count as given when their value differs from the default. +- `--cache_prefetch_concurrency` and `--cache_prefetch_timeout` also have no + effect with `--no_prefetch`, since they only tune pre-fetching. + (`--cache_dir` is still used: verification reads the cache.) +- `--pull-all-apks` together with `--pull-preinstalled-apks-only` pulls only + pre-installed APKs. + +`--perform_inclusion_proof_check` without `--verifier_path` remains an error. + ### Machine-Readable Progress (`--events`) Log messages are meant for people and may change between versions. Wrappers diff --git a/uraniborg/scripts/python/automate_observation.py b/uraniborg/scripts/python/automate_observation.py index 9a1b291..d26e338 100644 --- a/uraniborg/scripts/python/automate_observation.py +++ b/uraniborg/scripts/python/automate_observation.py @@ -145,6 +145,54 @@ def parse_arguments() -> argparse.Namespace: return args +def validate_argument_combinations(args: argparse.Namespace) -> list[str]: + """Finds flags that are ignored because of how they are combined. + + These combinations are not errors, so that existing invocations keep + working; run() logs each returned message as a warning. Combinations that + cannot work at all are rejected by parse_arguments() instead. + + Args: + args: Parsed arguments from parse_arguments(). + + Returns: + One human-readable warning per ignored flag or conflict, in a stable + order. Empty if nothing is ignored. + """ + warnings = [] + # Integer flags always have a value, so only a non-default one counts as + # given. Only prefetch_log_entries() reads them. + prefetch_tuning = [ + ("--cache_prefetch_concurrency", + args.cache_prefetch_concurrency != + inclusion_proof_check.DEFAULT_PREFETCH_CONCURRENCY), + ("--cache_prefetch_timeout", + args.cache_prefetch_timeout != + inclusion_proof_check.DEFAULT_PREFETCH_TIMEOUT), + ] + if not args.perform_inclusion_proof_check: + ignored = [ + ("--check_preinstalled_only", args.check_preinstalled_only), + ("--verifier_path", args.verifier_path is not None), + ("--no_prefetch", args.no_prefetch), + ("--cache_dir", args.cache_dir is not None), + ] + prefetch_tuning + for flag, given in ignored: + if given: + warnings.append("{} has no effect without " + "--perform_inclusion_proof_check.".format(flag)) + elif args.no_prefetch: + # Pre-fetching is skipped, so its tuning flags are unused. --cache_dir is + # not: verification itself still uses the cache. + for flag, given in prefetch_tuning: + if given: + warnings.append("{} has no effect with --no_prefetch.".format(flag)) + if args.pull_all_apks is not None and args.pull_preinstalled_apks_only: + warnings.append("--pull-all-apks and --pull-preinstalled-apks-only were " + "both given; only pre-installed APKs will be pulled.") + return warnings + + def set_up_logging(args: argparse.Namespace) -> logging.Logger: """Sets up various logging parameters. @@ -1321,6 +1369,8 @@ def run(args: argparse.Namespace, logger: logging.Logger, return 0, as they always have; run_finished.error describes them. """ events.emit("run_started", argv=sys.argv[1:], pid=os.getpid()) + for warning in validate_argument_combinations(args): + logger.warning(warning) def early_exit(reason: str, message: str) -> int: events.finish_run(0, error=_error(reason, message)) diff --git a/uraniborg/scripts/python/tests/test_automate_observation.py b/uraniborg/scripts/python/tests/test_automate_observation.py index 781603d..f81f094 100644 --- a/uraniborg/scripts/python/tests/test_automate_observation.py +++ b/uraniborg/scripts/python/tests/test_automate_observation.py @@ -1876,5 +1876,129 @@ def test_main_without_serial_no_devices_keeps_early_return( m["logger"].error.assert_any_call("No devices connected!") +_NO_PROOF = "has no effect without --perform_inclusion_proof_check." +_BOTH_PULL = ("--pull-all-apks and --pull-preinstalled-apks-only were both " + "given; only pre-installed APKs will be pulled.") +_PROOF = ("--perform_inclusion_proof_check", "--verifier_path=/v") + + +def _warnings_for(monkeypatch: pytest.MonkeyPatch, *extra: str) -> list[str]: + _set_argv(monkeypatch, *extra) + return automate_observation.validate_argument_combinations( + automate_observation.parse_arguments()) + + +@pytest.mark.parametrize( + "extra", + [ + (), + _PROOF, + (*_PROOF, "--check_preinstalled_only", "--cache_dir=/c", + "--cache_prefetch_concurrency=4", "--cache_prefetch_timeout=30"), + # Verification still uses the cache when pre-fetching is off. + (*_PROOF, "--no_prefetch", "--cache_dir=/c"), + ("--pull-all-apks",), + ("--pull-preinstalled-apks-only",), + (*_PROOF, "--pull-all-apks"), + # Integer flags set to their defaults count as not given. + ("--cache_prefetch_concurrency={}".format( + inclusion_proof_check.DEFAULT_PREFETCH_CONCURRENCY), + "--cache_prefetch_timeout={}".format( + inclusion_proof_check.DEFAULT_PREFETCH_TIMEOUT)), + ], + ids=["defaults", "proof_check", "proof_check_all_options", + "no_prefetch_with_cache_dir", "pull_all", + "pull_preinstalled", "proof_check_and_pull_all", + "explicit_int_defaults"], +) +def test_validate_argument_combinations_no_warnings( + monkeypatch: pytest.MonkeyPatch, extra, +): + assert _warnings_for(monkeypatch, *extra) == [] + + +@pytest.mark.parametrize( + "extra, expected", + [ + (("--check_preinstalled_only",), + "--check_preinstalled_only " + _NO_PROOF), + (("--verifier_path=/v",), "--verifier_path " + _NO_PROOF), + (("--no_prefetch",), "--no_prefetch " + _NO_PROOF), + (("--cache_dir=/c",), "--cache_dir " + _NO_PROOF), + (("--cache_prefetch_concurrency=4",), + "--cache_prefetch_concurrency " + _NO_PROOF), + (("--cache_prefetch_timeout=30",), + "--cache_prefetch_timeout " + _NO_PROOF), + ((*_PROOF, "--no_prefetch", "--cache_prefetch_concurrency=4"), + "--cache_prefetch_concurrency has no effect with --no_prefetch."), + ((*_PROOF, "--no_prefetch", "--cache_prefetch_timeout=30"), + "--cache_prefetch_timeout has no effect with --no_prefetch."), + (("--pull-all-apks", "--pull-preinstalled-apks-only"), _BOTH_PULL), + ], + ids=["check_preinstalled_only", "verifier_path", "no_prefetch", + "cache_dir", "cache_prefetch_concurrency", "cache_prefetch_timeout", + "no_prefetch_concurrency", "no_prefetch_timeout", "both_pull_flags"], +) +def test_validate_argument_combinations_one_warning_each( + monkeypatch: pytest.MonkeyPatch, extra, expected, +): + assert _warnings_for(monkeypatch, *extra) == [expected] + + +def test_validate_argument_combinations_reports_all_in_stable_order( + monkeypatch: pytest.MonkeyPatch, +): + assert _warnings_for( + monkeypatch, "--pull-preinstalled-apks-only", "--pull-all-apks", + "--cache_dir=/c", "--check_preinstalled_only") == [ + "--check_preinstalled_only " + _NO_PROOF, + "--cache_dir " + _NO_PROOF, + _BOTH_PULL, + ] + + +def test_validate_argument_combinations_no_prefetch_with_both_tuning_flags( + monkeypatch: pytest.MonkeyPatch, +): + assert _warnings_for( + monkeypatch, *_PROOF, "--no_prefetch", "--cache_prefetch_concurrency=4", + "--cache_prefetch_timeout=30") == [ + "--cache_prefetch_concurrency has no effect with --no_prefetch.", + "--cache_prefetch_timeout has no effect with --no_prefetch.", + ] + # Without the proof check, each flag gets only the "without" warning. + assert _warnings_for( + monkeypatch, "--no_prefetch", "--cache_prefetch_timeout=30") == [ + "--no_prefetch " + _NO_PROOF, + "--cache_prefetch_timeout " + _NO_PROOF, + ] + + +def test_main_logs_argument_warnings_before_doing_any_work( + serial_main_mocks, monkeypatch: pytest.MonkeyPatch, +): + m = serial_main_mocks + m["supported_platform"].return_value = False # stop right after the checks + _set_argv(monkeypatch, "--no_prefetch", "--pull-all-apks", + "--pull-preinstalled-apks-only") + + automate_observation.main() + + assert m["logger"].warning.call_args_list == [ + mock.call("--no_prefetch " + _NO_PROOF), mock.call(_BOTH_PULL)] + + +def test_main_logs_no_argument_warnings_by_default( + serial_main_mocks, monkeypatch: pytest.MonkeyPatch, +): + m = serial_main_mocks + m["AdbWrapper"].devices.return_value = [_make_mock_device("DEV1")] + _set_argv(monkeypatch) + + automate_observation.main() + + m["logger"].warning.assert_not_called() + + if __name__ == "__main__": sys.exit(pytest.main([__file__]))