Support disabling code probes in test harness#13152
Support disabling code probes in test harness#13152tclinkenbeard-oai wants to merge 1 commit intoapple:mainfrom
Conversation
tclinkenbeard-oai
left a comment
There was a problem hiding this comment.
Generated by Codex.
What is it trying to do?
This adds a TH_DISABLE_CODE_PROBES / --disable-code-probes switch for TestHarness2 so Joshua agents can skip collecting per-test code coverage, avoid persisting coverage rows to FDB, and treat ensemble coverage checks as intentionally disabled when summarizing results.
Is it correct?
Yes, based on code inspection. The flag is wired through the relevant paths consistently:
summarize.pystops collectingCodeCoverageevents and skipswrite_coverage(...)results.pyskipsread_coverage(...), markscoverage_ok = True, and emitsCodeProbeTrackingEnabled="0"- the README and Joshua wrapper comments document the new knob
The _read_env() change is also directionally correct: the old bool("false") == True behavior meant existing boolean environment variables could not reliably be set to false. The new parser fixes that for all bool-backed TestHarness2 env vars, not just this new one.
I did not run build or test validation for this review. GitHub currently reports no status checks on the PR branch.
Are there bugs?
I did not find any correctness bugs.
Are there omissions?
There are no automated tests for either the new disabled-code-probes path or the new shared boolean environment parsing behavior. Because _parse_env_value() changes semantics for every bool-backed env var, a small config-focused test covering accepted true/false spellings and invalid values would be useful, though I do not think its absence blocks this PR.
Are there better ways of doing things?
The shared boolean parser is better than adding one-off handling for only TH_DISABLE_CODE_PROBES; it fixes an existing class of env parsing mistakes at the right layer. I do not see a materially better design needed for this change.
Should this CL be LGTMd?
Yes, LGTM. I inspected the changed config, summarization, results, and documentation paths, and the disabled path appears internally consistent. The main remaining risk is the lack of automated coverage for the new boolean parsing behavior and disabled-code-probes flow.
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
|
@ploxiln could you have a look at this one |
For the FDB cluster backing Joshua, persisting code probe data is the most expensive part of running a Joshua ensemble. In some cases, this is unnecessary, so this PR supports disabling code probe tracking via a
TH_DISABLE_CODE_PROBESenvironment variable on Joshua agents. Tested by running a Joshua ensemble with the environment variable set.