-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
main: move norecursedir check to main's pytest_ignore_collect #11082
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
Conversation
Fix pytest-dev#11081
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.
nice, this also brings the implementation and the config for it back together
Hi. I suspect this might have broken our test collection downstream at https://github.com/astropy/astropy . I see that it is collecting stuff that we explicitly ignored in
Despite that directive, I see:
Is there some changes I need to do downstream to account for this new pytest behavior? Thanks! Failed log: https://github.com/astropy/astropy/actions/runs/5191708098/jobs/9359883509 Last success log: https://github.com/astropy/astropy/actions/runs/5173838762/jobs/9319503854 |
Thanks for testing pytest main @pllim, it helps a lot. I don't have time to confirm this ATM, but this is almost certainly due to a plugin which "overrides"
To explain a bit, The The So I believe that if you change this line to Note that it will change the behavior such that pytest's builtin ignore logic will now run, when it didn't previously. If that's a problem, the only fix is to just copy over the If you can confirm the above that'd be great, I think it will be a good change to make regardless. However, I'll be taking a look at how plugins at large implement |
Thank you very much for the thorough analysis and recommendation, @bluetech ! I think your proposed solution looks very promising over at scientific-python/pytest-doctestplus#201 . 😺 |
Did this now. Out of the ~700 plugins I have checked out locally, 11 implement I also did a github search, there aren't many hits, but I'd say it's 10%-20% incorrect implementations. Overall my impression is that the breakage won't be wide-spread. The breakage will be visible and the fix is backward compatible - no compat issues. In addition to the fact that it is already subtlety broken. So I think we shouldn't revert. I will however add a note to the changelog entry about this. |
Fix #11081