Repository navigation
Skip the module-release test once core stops leaking; fix a comment - #2117
Merged
Merged
Conversation
CI installs the newest policyengine-core. If a core release fixes the sys.modules leak (policyengine-core#520), the regression test's precondition would fail on every PE-UK PR until someone removed the workaround. It now skips with a note to remove release_dead_variable_modules and the test. Also correct the conftest comment: 15.8 GB was the CI runner's memory in use while the suite ran serially, not one process's size. Both from the review of #2115. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From the review of this PR: skipping whenever no module carried the system's id would also skip if core kept leaking but changed the name format, and then the conftest pattern would silently release nothing. The test now looks up the module core loaded a variable from: it skips only if that module is gone from sys.modules, and otherwise asserts the conftest pattern still recognises it. release_dead_variable_modules now counts only modules it actually removed, so the test's `released >= 1` fails when nothing is removed (checked by disabling the removal). The conftest comment now gives the serial run's memory as 99% of the runner's, since free -m reports MiB. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
Merged at the reviewed head a77930f with a merge commit, on these gates:
This is test-only, so there is no changelog fragment and no release. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2115: the two code nits from its independent review.
test_variable_module_release.py: CI installs the newest policyengine-core. Once a core release fixes thesys.modulesleak (TaxBenefitSystem construction permanently leaks ~5,600 sys.modules entries (~55-60MB) per build — unreclaimable by gc policyengine-core#520), the preconditionassertwould fail every PE-UK PR until someone deleted the workaround. It now callspytest.skip, with a message saying to removerelease_dead_variable_modulesand the test. With today's core the test still runs and passes. With the fix commented out it still fails, because that check comes after the precondition.conftest.py: the comment said one test process grew to 15.7 GB. The measured figure was 15.8 GB of the runner's memory in use while the suite ran serially, and the comment now says that.These are test-only changes with no package or dependency change, so there is no changelog fragment and no release.
axiom: n/a: test infrastructure only.
🤖 Generated with Claude Code