-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
Test inside pytest_runtest_call
hook
#36
Test inside pytest_runtest_call
hook
#36
Conversation
@pllim I think the CI workflow is disabled |
This is to prevent a pytest warning or error
Let me close/reopen. |
Might have to drop Python 3.7 ? |
Could just ignore the warning? Which would you prefer? |
Ah, true, you could. |
Thanks. I don't grok the internals here, so I pinged the people who has release rights for review (https://pypi.org/project/pytest-arraydiff). |
#35 should only be an issue when pytest-arraydiff is either not installed or not enabled. I think it's also worth updating for the enabled case here because we found that the existing wrapper method didn't test class based tests. |
Do you plan to do that in this PR? If not, please open a new issue for it. Thanks! |
I already fixed that in the first commit. I was just justifying the change which I later realised wasn't necessary to fix the actual issue referenced. 😆 I've now confirmed that pytest-arraydiff doesn't work with class-based tests on recent pytest versions. You can see in the logs that it only shows the warning for the class test (because it wasn't able to wrap the test method for array testing): https://github.com/astropy/pytest-arraydiff/actions/runs/2914788915/jobs/4644034561#step:5:148 I tried modifying the return value of |
If you don't hear back from the actual package maintainers, try poke them on Astropy Slack. Thanks! |
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.
minor comment only, you need Tom to sign off on the actual code changes
4537f35
to
9ffd246
Compare
I will try and review this shortly. I do wonder if there would be scope to develop a pytest plugin that deals with getting return values that both pytest-mpl and this plugin could depend on to avoid duplicating infrastructure? |
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.
Looks good to me, thanks!
This changes the plugin's pytest integration to be the same as pytest-mpl, which is able to handle intercepting returned values for further testing.
Fix #35