Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve assert mod not in mods error message #11795

Merged

Conversation

lesteve
Copy link
Contributor

@lesteve lesteve commented Jan 10, 2024

Related to #9765, add a better error message than assert mod not in mods. This would have been super useful to figure out from the error message the difference from conftestpath and mod.__file__. Hopefully the #11708 will be merged and this error is less likely to occur, but at least if a variation of the issue occurs again there will be a better error message.

Here is the error on my reproducer https://github.com/lesteve/pytest-issue-9765-reproducer:

AssertionError: str(conftestpath)='C:\\msys64\\home\\lesteve\\dev\\pytest-issue-9765-reproducer\\project\\conftest.py'
has already been loaded as the module mod=<module 'project.conftest' from 'c:\\msys64\\home\\lesteve\\dev\\pytest-issue
-9765-reproducer\\project\\conftest.py'>

For now I keep an AssertionError but let me know if you think another kind of error is more appropriate.

It does not seem that easy to add a test for this ...

Let me know if you think this is worth adding a changelog for this or not!

Depending on how #11708 goes, this may need to be tweaked since a discrepancy between mod.__file__ and str(contestpath) is likely not how this problem will show up. It could even lead to red-herring, since it will be potentially fine that mod.__file__ and str(conftestpath) are not the consistent and people may be misled by reading comments like #9765 (comment) ...

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed having a better error message is something we should have for all internal assert calls, thanks!

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this would have been useful with the issue.

@@ -656,7 +656,8 @@ def _importconftest(
if dirpath in self._dirpath2confmods:
for path, mods in self._dirpath2confmods.items():
if dirpath in path.parents or path == dirpath:
assert mod not in mods
error_message = f"{str(conftestpath)=} has already been loaded as the module {mod=}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it will be useful to include the __file__ as a debugging aid - it may no longer be at fault but it's useful to see where mod came from.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this update

[ran: tweaked message, made the formatting lazy]
@bluetech bluetech force-pushed the improve-assert-mod-not-in-mods-error-message branch from be9d154 to 1c9d683 Compare January 14, 2024 11:22
@bluetech bluetech merged commit 2bb0eca into pytest-dev:main Jan 14, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants