fix(pytest): fix failing unit test test_model_copy[Environment]#1711
fix(pytest): fix failing unit test test_model_copy[Environment]#1711felix314159 wants to merge 2 commits into
test_model_copy[Environment]#1711Conversation
marioevz
left a comment
There was a problem hiding this comment.
I think this is more related to how we use EnvironmentDefaults rather than the copy, because we rely on exclude_unset to later be able to use model_fields_set to peek into what the tester really wanted to specify.
I think execute.py and filler.py should be changed, it is kinda insane if i understand that correctly. They both modify default values, why would you overwrite a default ever? You are supposed to derive something else from the default and leave the default untouched. But also, what even happens if you both specify custom values for |
danceratopz
left a comment
There was a problem hiding this comment.
Thanks for taking a look at this @felix314159! This fix resolves the unit test fail, but breaks filling.
To be clear, this fail is flaky and not deterministically reproducible. Locally in a loop, I hit it 50% of the time. As such, it's sporadically interfering with CI, so we should prioritize a fix for this, even if it doesn't solve the underlying architectural issue.
This fail points to a code smell and I think we need to take a deeper look. In the meantime, a hotfix seems necessary to keep CI clean.
Claude helped me come up with a (hot)fix, I'll push it to this branch, so we can keep the discussion here and get a CI run in.
| """ | ||
| return self.__class__(**(self.model_dump(exclude_unset=True) | kwargs)) | ||
| # Get all current field values, including those set via default_factory | ||
| dump_dict = self.model_dump() |
There was a problem hiding this comment.
We need to ensure that we exclude unset fields, or we have a problem with mutually exclusive fields, e.g. v, r, s are either explicitly set OR computed, so these need to be excluded from the copy to avoid a conflict with secret_key.
7fabc6b to
6b734c8
Compare
6b734c8 to
e254fcc
Compare
test_model_copy[Environment]
|
I addressed the issue with filling in this PR, Claude wrote a follow-up for me, I couldn't have explained it better 😆 |
|
Alternative fix to the pressing CI problem here: |
* feat(tests): add initial eip-7825 test cases. * chore: add exceptions mapper, clean up, ci fix. * chore(docs): changelog. * chore: spellcheck. * chore: fix coverage ci. --------- Co-authored-by: spencer-tb <spencer@spencertaylorbrown.uk>
* feat(tests): add initial eip-7825 test cases. * chore: add exceptions mapper, clean up, ci fix. * chore(docs): changelog. * chore: spellcheck. * chore: fix coverage ci. --------- Co-authored-by: spencer-tb <spencer@spencertaylorbrown.uk>
* feat(tests): add initial eip-7825 test cases. * chore: add exceptions mapper, clean up, ci fix. * chore(docs): changelog. * chore: spellcheck. * chore: fix coverage ci. --------- Co-authored-by: spencer-tb <spencer@spencertaylorbrown.uk>
🗒️ Description
AI slop below
Problem
The
test_model_copyunit test at line 890 oftest_types.pywas failing because:Environmentmodel has agas_limitfield with adefault_factorythat referencesEnvironmentDefaults.gas_limitEnvironmentDefaults.gas_limitgloballyCopyValidateModel.copy()method usedmodel_dump(exclude_unset=True), which excludes fields set viadefault_factorydefault_factorywould run again with the new (modified) value, resulting in differentgas_limitvalues between the original and the copySolution
Modified the
copy()method inpackages/testing/src/execution_testing/base_types/pydantic.pyto:model_dump()withoutexclude_unset=Trueto capture all actual field values (including those set viadefault_factory)model_fields_setby setting__pydantic_fields_set__to preserve which fields were explicitly set vs. set by defaultsThis ensures that:
model_fields_setis maintained correctly (satisfying the test assertion)🔗 Related Issues or PRs
N/A.
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture