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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we understand why does sys.modules.values() change during iteration? Is it a side-effect of pytest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, though I don't understand why it sys.modules changes in this loop (assuming no background threads). I believe that accessing
m.__dict__
triggers a lazy loading of the module.I tested adding an additional for-loop before the original. The first loop iterates on list(sys.modules.value()), while the second on sys.modules.value(). The first loop conditionally accesses m.dict to see which modules change sys.modules. If the second loop succeeds, the touched modules are culprits.
I've isolated this module as a culprit:
py._vendored_packages.apipkg.ApiModule
Maybe related:
pytest has a module import hook for rewriting assertions (no need for self.assertEqual(), etc.):
https://github.com/pytest-dev/pytest/blob/7dcd9bf5add337686ec6f2ee81b24e8424319dba/src/_pytest/assertion/rewrite.py#L277-L301
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/pytest-dev/py/tree/master/py/_vendored_packages
https://github.com/pytest-dev/py/blob/master/py/__init__.py
So an alternative solution would be to add an additional loop before the original one:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robertwb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charlesccychen I believe my above suggestion is the more correct one, allowing lazily-loaded modules to initialize before putting their contents in known_module_dicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @udim--that makes sense. Are you going to update this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charlesccychen updated, ready to merge once all tests pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
list()
essential in line 168? If so please add a comment so that somebody doesn't try to clean it up, otherwise we can remove it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, added more commentary