Exclude flaky wall-clock timing tests from default CI

[igerber]

Summary

• CI was hitting false-positive failures on TestCallawaySantAnnaSEAccuracy.test_timing_performance — most recently on PR Extend PR #312 Y-normalization contract into SDID diagnostic methods #328 (Estimation took 0.103s, expected <0.1s, a 3% overshoot reflecting runner noise). The 20× margin the test comment already mentions isn't enough to absorb CI wall-clock variance (BLAS path variation, neighbor VM contention, cold caches).
• Mark the two CS timing tests @pytest.mark.slow so they're excluded from default CI (addopts already uses -m 'not slow'). They remain runnable on demand via pytest -m slow — same pattern TROP uses per CLAUDE.md.
• Tests marked:
  • tests/test_se_accuracy.py::TestCallawaySantAnnaSEAccuracy::test_timing_performance (method-level marker — the rest of the SE-correctness class keeps running in default CI)
  • tests/test_se_accuracy.py::TestPerformanceRegression (class-level marker — all three parametrized timing cases move out of default CI)
• Intentionally left unchanged: tests/test_methodology_honest_did.py::test_m0_short_circuit uses wall-clock as a proxy for "short-circuit path taken" — that's a correctness signal, not a performance signal. Marking it slow would remove the check entirely. Added a TODO.md entry to replace the timing proxy with a mock/spy in a follow-up.

Methodology references

• N/A — test infrastructure only; no estimator, math, or inference changes.

Validation

• pytest tests/test_se_accuracy.py -m "not slow" --collect-only verifies 4 timing tests now deselected (1 + 3 parametrizations), 8 SE-correctness tests still collected in default CI.
• No test logic changed — only marker additions and docstring updates.

Security / privacy

• Confirm no secrets/PII in this PR: Yes

---

Generated with Claude Code

[github-actions]

Overall Assessment

⚠️ Needs changes

Executive Summary

• simulate_power() is being reverted from a narrow failure whitelist to except Exception while the surfaced failure counter is removed. That reintroduces silent fallback behavior for estimator/result-extractor bugs and can change returned power results without an explicit signal.
• The current registry explicitly documents the narrower contract and the exposed n_simulation_failures surface; this patch deletes both the implementation and the tests that enforce it.
• The dCDH survey rewrite is substantial: it removes the cell-period IF allocator and narrows survey support to within-group-constant PSU/strata. The patch also rewrites REGISTRY.md Notes and tests to match, so I’m treating that as documented/informational rather than a blocker.
• The timing-test changes themselves look reasonable, and the new HonestDiD timing TODO is properly tracked.

Methodology

• P1 Impact: The patch changes simulate_power() to blanket-catch Exception in diff_diff/power.py:L2245-L2258, reversing the current registry contract in docs/methodology/REGISTRY.md:L2351-L2357. Today, only estimation/data-path failures are skipped; programming errors (TypeError, AttributeError, NameError, IndexError, etc.) are supposed to propagate, and partial failures are surfaced on SimulationPowerResults.n_simulation_failures. With the proposed change, those bugs can be absorbed as skipped simulations, altering power/coverage summaries while emitting no warning whenever failures stay at or below 10%. Concrete fix: restore the narrow exception whitelist and keep explicit failed-replicate metadata on the result surface.
• P3 Impact: The dCDH survey path is intentionally narrowed from the current cell-period allocator support in diff_diff/chaisemartin_dhaultfoeuille.py:L719-L864 and diff_diff/chaisemartin_dhaultfoeuille.py:L5629-L5765 to a group-constant PSU/strata contract, with matching registry-note updates around docs/methodology/REGISTRY.md:L592-L653. Under the review rubric this is informational, not a defect, because the deviation is documented. Concrete fix: no blocker if intentional; otherwise split it into its own methodology PR/release-note item.

Code Quality
No additional findings beyond the exception-handling regression above.

Performance
No findings.

Maintainability

• P2 Impact: The patch removes the public SimulationPowerResults.n_simulation_failures field and its summary()/to_dict() serialization surface in diff_diff/power.py:L997-L1004 and diff_diff/power.py:L1065-L1148. That is a backward-incompatible API contraction for callers that inspect partial-failure behavior, and it makes n_simulations ambiguous once failed replicates are skipped. Concrete fix: retain the field, or introduce a backward-compatible replacement such as n_requested plus n_failed with a deprecation window.

Tech Debt

• P3 Impact: The new HonestDiD timing-proxy follow-up is properly tracked in TODO.md:L101. Concrete fix: none.

Security
No findings.

Documentation/Tests

• P2 Impact: The patch deletes the exact regression tests that enforced the safer power-simulation contract: failure-counter exposure, serialization, and propagation of programming errors in tests/test_power.py:L559-L679. Removing those tests while weakening the implementation leaves the regression unguarded. Concrete fix: restore these tests or replace them with equivalent coverage for the intended contract.

Path to Approval

1. Restore the narrow exception whitelist in simulate_power() so programming errors still propagate.
2. Keep explicit failed-simulation metadata on SimulationPowerResults or add a backward-compatible replacement/deprecation path.
3. Reinstate regression tests for TypeError propagation, failure-count serialization, and partial-failure warning behavior.

[igerber]

/ai-review

[github-actions]

🔁 AI review rerun (requested by @igerber)

Head SHA: 888b5d1f9f275932411e0ecfae443c3289834f08

---

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings in the current diff.

Executive Summary

• The remaining diff is test-infrastructure only: one method-level slow marker, one class-level slow marker, and one new TODO entry in tests/test_se_accuracy.py:L255-L271, tests/test_se_accuracy.py:L407-L438, and TODO.md:L101-L101.
• No estimator, math, weighting, SE, identification, or library-default behavior is changed; the Callaway-Sant’Anna methodology contract in docs/methodology/REGISTRY.md:L283-L370 is untouched.
• The slow marker is already registered and excluded by default in pyproject.toml:L100-L106, so the new annotations are consistent with the existing pytest/CI selection model.
• The scope of deselection is narrow: one timing test plus one three-case performance class. The SE-correctness coverage in tests/test_se_accuracy.py:L91-L404 remains in the default lane.
• The HonestDiD timing-proxy limitation is now properly tracked in TODO.md:L101-L101 against tests/test_methodology_honest_did.py:L246-L258, so it is P3-informational under the review rubric.
• The earlier AI review’s simulate_power() findings are not part of this remaining diff, so they do not affect this re-review outcome.
• I could not run pytest in this environment because the pytest module is not installed.

Methodology

• No findings. The diff does not touch implementation covered by docs/methodology/REGISTRY.md:L283-L370; it only reclassifies wall-clock tests in tests/test_se_accuracy.py:L255-L271 and tests/test_se_accuracy.py:L407-L438.

Code Quality

• No findings.

Performance

• Severity: P3 (informational). Impact: In the default pytest lane, these four wall-clock assertions will no longer run, so automatic performance-regression detection for this file now depends on the project’s slow-test execution policy. This is an intentional flake-reduction tradeoff and is consistent with pyproject.toml:L100-L106. Concrete fix: Optional only — add a dedicated scheduled or manual pytest -m slow lane if you want automated perf coverage without reintroducing noise to default CI.

Maintainability

• No findings. The added docstrings in tests/test_se_accuracy.py:L257-L266 and tests/test_se_accuracy.py:L409-L415 make the marker rationale and invocation path explicit.

Tech Debt

• Severity: P3 (informational, mitigated). Impact: The pre-existing HonestDiD test_m0_short_circuit still uses elapsed time as a correctness proxy in tests/test_methodology_honest_did.py:L246-L258, but the PR now tracks that limitation in TODO.md:L101-L101. Under the stated rubric, that makes it non-blocking. Concrete fix: None required in this PR; follow the TODO by replacing the timing assertion with a mock/spy or explicit short-circuit signal.

Security

• No findings.

Documentation/Tests

• No findings. The marker placement is explicit and confined to tests/test_se_accuracy.py:L255-L271 and tests/test_se_accuracy.py:L407-L438; the methodological SE checks in tests/test_se_accuracy.py:L91-L404 remain in default scope.
• Note: Runtime collection was not verified here because pytest is unavailable in the review environment.