Repository navigation
Make Hypothesis draw the same examples from the source tree and the wheel - #2400
Conversation
vahid-ahmadi
left a comment
There was a problem hiding this comment.
Fine once rebased down to the Hypothesis fix. Main already has the rest.
@anth-volk, #2399 merged to main on 14 Oct and shipped in 2.130.1. It makes the same change to is_benefit_cap_exempt_earnings.py (summing uc_individual_earned_income_before_mif), removes the #1986 strict xfail in test_uc_income_from_capital_properties.py, and pins the sick-pay family with @example in test_uc_statutory_pay_properties.py. That is why this branch now conflicts.
- should: rebase down to the part main doesn't have:
_ignore_hypothesis_local_constantsinpolicyengine_uk/tests/conftest.py, the guard testpolicyengine_uk/tests/test_hypothesis_environment.py, and the second line of the changelog fragment. On main,conftest.pyhas no Hypothesis change and the guard test doesn't exist, so source-tree checks and Release test can still draw different examples. That was the cause of #2397. #2399 closed the issue, but the cause is still on main.benefit_cap_earnings.yamlcan go: #2399 addsbenefit_cap_earnings_exception_earned_income.yaml, which covers statutory sick, maternity and paternity pay, tax on a private pension and the MIF. - nit, non-blocking: emptying the local-constant pool makes the two environments agree, but it also stops Hypothesis biasing draws towards the model's own thresholds (such as
10_152), and that bias is what found this bug. A later change could add those thresholds back on purpose, for example@examplecases each side of the cap earnings threshold.
What I checked
- The PR head is
fd9a0eee9, with no rebase since 13 Oct. GitHub reports it as conflicting with main. - On
origin/main:is_benefit_cap_exempt_earnings.pyusesuc_individual_earned_income_before_mif, the #1986 xfail is gone, andtest_uc_statutory_pay_properties.pyhas the@examplepin.conftest.pyhas no local-constants change, andtest_hypothesis_environment.pydoesn't exist. Hypothesis is still locked at 6.169.0. - I did not rerun the tests on this head; this note is about scope only.
🤖 Generated with Claude Code
Rebased onto main after #2399, which made the benefit cap earnings fix. Only the test-environment change remains: conftest stops Hypothesis drawing literal constants from policyengine_uk's own modules, and a guard test fails if a Hypothesis upgrade bypasses that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fd9a0ee to
5d670bd
Compare
juaristi22
left a comment
There was a problem hiding this comment.
Reviewed 5d670bd778927e68c8e9bfc6651510253de03ea9. Approved the narrowed test-environment fix: the diff contains the Hypothesis local-constant hook, a provider-level guard and changelog; the earlier benefit-cap changes are covered by #2399.
The provider reads the replaced getter and its cache is cleared before property tests run. Independent controls verify that the exact guard fails without the hook (1,506 source literals), passes with it, stays empty after later imports, and raises clearly if the private getter disappears. Source/site-packages classification and the CI source/wheel workflow were checked. The documented loss of automatic threshold-literal bias is an explicit trade-off; the relevant tests retain deliberate boundary samples and examples.
Validation: 15 focused tests and 11 controls passed. Local execution used Hypothesis 6.168.2/Core 3.32.27; the lock's Hypothesis 6.169.0 is covered by exact-head hosted CI. A built-wheel execution was not repeated locally. Reproducer and Axiom n/a records are appropriate. No findings or material evidence gaps.
Review status: COMPLETE. Review severity: APPROVE.
vahid-ahmadi
left a comment
There was a problem hiding this comment.
@anth-volk round 2, at 5d670bd77. I looked at what changed since fd9a0eee9, which here means the whole narrowed diff.
Verdict: the should is fixed. Approving.
Round-1 points
- Rebase down to the Hypothesis fix: fixed. Against main the branch is now three files and 45 lines:
_ignore_hypothesis_local_constantsintests/conftest.py, the guardtests/test_hypothesis_environment.py, and a one-line changelog fragment. The model change, the@examplepin, the #1986 xfail removal andbenefit_cap_earnings.yamlare all gone, as #2399 covers them. It now merges cleanly with current main (8b12bce94). - nit, loss of threshold bias: recorded. The description now sets this out as the trade-off, with deliberate
@exampleboundary cases as the way back. That's fine.
New findings
None. One cosmetic point: the fragment is still named fix-benefit-cap-earnings-statutory-pay.fixed.md. Only its text reaches the changelog, so it can stay.
What I checked
- Built a merge of this head into current main and ran it in a fresh uv venv, with Hypothesis 6.169.0 as locked on main.
- The guard plus
test_uc_statutory_pay_properties.pyandtest_uc_income_from_capital_properties.py: 15 passed. - Control: with the
pytest_configurecall removed, the guard fails with 1,520 local constants on current main. - CI at this head ran against main as it was on 15 Oct, and main is now 266 commits further on. The hook changes the examples every property test draws, so I ran all 80
test_*propert*.pyfiles on the merge: 573 passed, 1 xfailed. - No other open PR touches
tests/conftest.py. - CI is green at this head.
- Not checked: I didn't build a wheel and compare its draws side by side with the source tree. I'm relying on Hypothesis never counting site-packages as local, and on the guard showing the source-tree pool is empty.
🤖 Generated with Claude Code
|
Merged by the UK hub (Claude session 8e19f9d5) at head
|
Follow-up to #2397, rebased onto main after #2399.
What and why
#2399 fixed the benefit cap earnings exemption and unblocked releases. This PR is now only the test-environment half of #2397: why pull request checks passed a commit that Release test failed.
Hypothesis 6.169 biases its draws with literal constants harvested from local modules (
hypothesis/internal/conjecture/providers.py: _get_local_constants), and never counts a path under/site-packages/as local (constants_ast.py: is_local_module_file). From the source tree it collects 37 integers, 51 floats and 1,301 strings frompolicyengine_uk, including thresholds such as10_152; from the installed wheel, none. With the samederandomizeseed, the two environments generate different examples, so a property-test failure can show up only at release. Main still has this gap: #2399 pinned the one family that failed, but nothing stops the same divergence on any other property test.policyengine_uk/tests/conftest.py:pytest_configurenow empties Hypothesis's local-constant pool (_ignore_hypothesis_local_constants), and raises if the private function it replaces is gone.policyengine_uk/tests/test_hypothesis_environment.py: reads the pool throughHypothesisProvider._local_constants, as Hypothesis does, and fails if a Hypothesis upgrade brings the local constants back.The earlier version of this branch also carried the model fix, the
@examplepin, the #1986 xfail removal andbenefit_cap_earnings.yaml; all of that is dropped, as #2399 covers it.Trade-off (from review): with no local constants, Hypothesis no longer aims draws at the model's own thresholds, which is what found this bug. That can be added back deliberately later, for example with
@examplecases each side of the cap earnings threshold.Test plan
uv sync --frozen --extra dev, Python 3.13) at 5d670bd: the guard test,test_uc_statutory_pay_properties.pyandtest_uc_income_from_capital_properties.py(15 passed).ruff format --checkclean on the changed files.changelog.d/Reproducer
Reproducer: policyengine_uk/tests/test_hypothesis_environment.py::test_hypothesis_draws_no_constants_from_local_modules
axiom: n/a: test configuration only; no policy change.
🤖 Generated with Claude Code