Modularize tests (#15149)

[rban1]

Summary: Pull Request resolved: #15149

Differential Revision: D117393982

[meta-codesync]

@rban1 has exported this pull request. If you are a Meta employee, you can view the originating Diff in D117393982.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 0.0s.

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 5ddfacd

---

Summary

Clean infrastructure change that modularizes crash test targets and replaces a hardcoded 900s overrun grace with a dynamic, duration-proportional one. No production code touched; behavioral backward compatibility preserved for existing targets.

High-severity findings (0):
No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. liveness_crash_test not updated to use CRASHTEST_BB or CRASHTEST_WB — crash_test.mk:99

• Issue: All other leaf targets were migrated from $(CRASHTEST_PY) to $(CRASHTEST_BB) or $(CRASHTEST_WB), but liveness_crash_test still uses the bare $(CRASHTEST_PY), which does not pass --duration=. This means liveness uses its own default (DEFAULT_LIVENESS_TIMEOUT_SEC = 3600), which is fine functionally, but the inconsistency means a CI system overriding BLACKBOX_DURATION/WHITEBOX_DURATION cannot influence liveness runtime via the same mechanism.
• Root cause: Liveness has its own duration logic (liveness_default_timeout_sec()), so this may be intentional. If so, a brief comment would help future readers.
• Suggested fix: Either add a comment explaining the intentional omission, or introduce a LIVENESS_DURATION variable for consistency.

🟢 LOW / NIT

L1. Comment in Makefile references whitebox_crash_main without file context — crash_test.mk:25

• Issue: The comment "whitebox can overrun by up to a tenth of it (see whitebox_crash_main)" is helpful but would be clearer with a file reference (tools/db_crashtest.py).
• Suggested fix: Append (tools/db_crashtest.py) to the parenthetical.

L2. WHITEBOX_OVERRUN_GRACE_FRACTION = 10 naming — db_crashtest.py:51

• Issue: The name suggests the grace IS 1/10 of the duration, but it is actually the denominator in duration // WHITEBOX_OVERRUN_GRACE_FRACTION. The naming is fine for a constant, but the comment says "Scaled to --duration rather than flat" — the word "fraction" in combination with integer division might momentarily confuse a reader. Extremely minor.
• Suggested fix: No action needed; the code is clear enough in context.

Cross-Component Analysis

Concern	Analysis	Result
Backward compat for blackbox_crash_test	Aggregate target now invokes leaf targets via $(CRASHTEST_MAKE) recursion. Each leaf uses $(CRASHTEST_BB) which passes --duration=6000 (matching Python's blackbox_default_params["duration"] = 6000). Behavior unchanged.	Safe
Backward compat for whitebox_crash_test	Same pattern, --duration=10000 matches whitebox_default_params["duration"] = 10000.	Safe
CI override via CRASH_TEST_EXT_ARGS	CI passes --duration=240 via CRASH_TEST_EXT_ARGS. Since $(CRASH_TEST_EXT_ARGS) appears AFTER --duration=$(BLACKBOX_DURATION), argparse takes the last value. Override works correctly.	Safe
Overrun grace with short durations	With --duration=240, grace = max(300, min(900, 240//10)) = max(300, 24) = 300s. Previously 900s. The 300s minimum is generous for a 240s run.	Safe
make -j parallelism	Aggregate targets use $(CRASHTEST_MAKE) sequential invocations with # Do not parallelize comments. Leaf targets are independent and can be parallelized by CI. Correct.	Safe
.PHONY declarations	New leaf targets simple_blackbox_crash_test, std_blackbox_crash_test, simple_whitebox_crash_test, std_whitebox_crash_test are added to .PHONY.	Correct

Positive Observations

• The modularization is well-structured: leaf targets do exactly one db_crashtest.py invocation, aggregate targets compose them sequentially. This cleanly enables CI parallelism.
• The dynamic overrun grace is a thoughtful improvement over the flat 900s — it prevents short CI runs from wasting most of their budget on a single overrunning iteration.
• Default durations (WHITEBOX_DURATION=10000, BLACKBOX_DURATION=6000) are chosen to match the Python defaults, so behavior is unchanged unless explicitly overridden.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit c6854cc

---

Summary

Clean, low-risk refactor that splits monolithic crash test targets into individually-schedulable leaf targets while preserving backward-compatible aggregates.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

None.

🟢 LOW / NIT

L1. Consider adding the new leaf targets to CI workflows — .github/workflows/pr-jobs.yml:419-421

• Issue: The stated purpose is to let CI schedule leaf targets as separate jobs, but the CI workflows (.github/workflows/pr-jobs.yml, nightly.yml) still reference only blackbox_crash_test (the aggregate). The Makefile change is a prerequisite, but without corresponding CI workflow updates, the parallelism benefit is not realized in open-source CI.
• Note: This may be intentional if the leaf targets are consumed by Meta-internal CI only. If so, no action needed.

Cross-Component Analysis

Context	Affected?	Notes
Open-source CI (GitHub Actions)	No behavior change	blackbox_crash_test aggregate still works identically
Meta-internal CI	Enabled	Can now schedule simple_* and std_* as separate jobs
Local make runs	No behavior change	Aggregate targets still run both sub-tests sequentially
Makefile ASAN/UBSAN targets	No behavior change	They invoke whitebox_crash_test/blackbox_crash_test (lines 1312-1344) which now delegate to leaf targets via $(CRASHTEST_MAKE). Since Makefile includes crash_test.mk, the delegation chain works correctly.

Positive Observations

• Follows the exact same delegation pattern already used by crash_test (line 47-50), ensuring consistency.
• Backward compatibility is fully preserved — existing make blackbox_crash_test and make whitebox_crash_test invocations work identically.
• Minimal, well-scoped change touching only the single relevant file.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 6c1f8d4

---

Summary

Clean, low-risk Makefile refactoring that splits monolithic blackbox_crash_test and whitebox_crash_test targets into individual leaf targets (simple_blackbox_crash_test, std_blackbox_crash_test, etc.) to allow CI systems to schedule them as separate jobs. The aggregate targets are preserved for backwards compatibility. The export CRASH_TEST_EXT_ARGS addition correctly ensures the variable propagates through recursive make invocations.

High-severity findings (0):
No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Missing .PHONY entries may be incomplete — crash_test.mk:29

• Issue: The new leaf targets simple_blackbox_crash_test, std_blackbox_crash_test, simple_whitebox_crash_test, and std_whitebox_crash_test are added to the .PHONY list (line 29-30 in the diff), which is correct. However, they are placed on the same line as liveness_crash_test rather than on their own continuation line, making the list harder to read and maintain. This is a style nit rather than a functional issue.
• Root cause: Formatting choice.
• Suggested fix: Consider placing each new target on its own continuation line for consistency with the rest of the .PHONY declaration.

🟢 LOW / NIT

L1. export CRASH_TEST_EXT_ARGS exports an unset variable when used standalone — crash_test.mk:19

• Issue: When crash_test.mk is used directly (make -f crash_test.mk ...) without CRASH_TEST_EXT_ARGS being set (either on the command line or from the parent Makefile's TSAN block at Makefile:412), export CRASH_TEST_EXT_ARGS exports an empty variable. This is harmless (the $(CRASH_TEST_EXT_ARGS) expansions just produce empty strings), but worth noting for clarity.
• Root cause: The export is needed for the recursive $(CRASHTEST_MAKE) calls introduced by this PR. Previously, when blackbox_crash_test ran both invocations as direct recipe lines, the variable expanded in the same make process. Now with recursive make, the variable must be exported. This is correct.
• Suggested fix: No change needed; the behavior is correct. The export is a no-op when the variable is empty.

L2. Comment style: "Do not parallelize" as a Makefile comment in recipe — crash_test.mk:107,163

• Issue: The # Do not parallelize comment appears between the target line and the first recipe command. In GNU Make, a # line in the recipe block is passed to the shell as a comment (harmless but slightly unconventional). This matches the existing pattern used for other aggregate targets (e.g., crash_test at line 48), so it is consistent.
• Root cause: Existing convention in this file.
• Suggested fix: No change needed; follows existing pattern.

Cross-Component Analysis

Context	Affected?	Assessment
Open-source CI (pr-jobs.yml)	YES — currently uses blackbox_crash_test as a matrix target	Safe. The aggregate blackbox_crash_test target still works, just delegates to leaf targets now. No CI changes needed.
Nightly CI (nightly.yml)	NO — uses specific targets like blackbox_crash_test_with_atomic_flush	Unaffected.
Internal Meta CI	YES — this is the primary consumer of the new leaf targets	Intended use case.
make -f crash_test.mk direct invocation	YES	Safe. export CRASH_TEST_EXT_ARGS ensures variable propagation through recursive make.
Makefile TSAN block (Makefile:412)	YES — sets CRASH_TEST_EXT_ARGS += --max_key=1000000	Safe. The export at line 19 of crash_test.mk ensures this propagates to sub-make processes. When included from Makefile, the variable is set before crash_test.mk is included (line 1301), so the export captures the correct value.

Positive Observations

• Backwards compatibility preserved: The aggregate targets (blackbox_crash_test, whitebox_crash_test) continue to work identically, just delegating to the new leaf targets.
• Correct use of export: The export CRASH_TEST_EXT_ARGS is the right solution for propagating the variable through $(CRASHTEST_MAKE) recursive invocations. Without it, the leaf targets called via $(CRASHTEST_MAKE) would lose any CRASH_TEST_EXT_ARGS set by the parent Makefile (e.g., TSAN's --max_key=1000000).
• Sequential execution preserved: The aggregate targets use $(CRASHTEST_MAKE) (not + or parallel markers), maintaining the sequential execution semantics that the # Do not parallelize comments document.
• Clean separation: Each leaf target runs exactly one db_crashtest.py invocation, making CI scheduling straightforward.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase