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

Bugfix: Fix tests not asserting error expected #464

Merged
merged 3 commits into from
Mar 21, 2023

Conversation

SeanLeRoy
Copy link
Collaborator

@SeanLeRoy SeanLeRoy commented Mar 14, 2023

How did this come about?

This came about from this issue where it was noted that AICSImage could not instantiate with an array-like image which greater than 5 dimensions. I was able to get around this issue (and also provide a fix in a related follow-up PR here), but while investigating I noticed we had a test case for this already and it was passing even though it shouldn't based on manual testing. This led me to believe this was an evergreen test and going from using pytest.mark.raises to using the xfail mark (like so pytest.mark.xfail) caused the test to fail with a message saying the test did not raise the expected error which was the case in my manual testing.

What this PR aims to accomplish

This pull request modifies all ~115 usages of pytest.mark.raises to pytest.mark.xfail which caused 5 tests to fail all of which were in the ArrayLikeReader tests. In my follow-up PR I mentioned above I fix the failing tests and resolve the issue aforementioned as well. The goal of this PR is not to merge this without the follow-up PR included to fix the tests, but rather just highlight and display these changes without cluttering the more important to review fixes.

Testing

Unit tested locally

@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2023

Codecov Report

Patch coverage: 50.00% and project coverage change: -27.48 ⚠️

Comparison is base (6b02c2f) 94.03% compared to head (49a09a3) 66.55%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #464       +/-   ##
===========================================
- Coverage   94.03%   66.55%   -27.48%     
===========================================
  Files          48       48               
  Lines        4155     4162        +7     
===========================================
- Hits         3907     2770     -1137     
- Misses        248     1392     +1144     
Impacted Files Coverage Δ
...ts/readers/extra_readers/test_bioformats_reader.py 96.72% <ø> (ø)
...eio/tests/readers/extra_readers/test_czi_reader.py 100.00% <ø> (ø)
...tests/readers/extra_readers/test_default_reader.py 100.00% <ø> (ø)
...geio/tests/readers/extra_readers/test_dv_reader.py 100.00% <ø> (ø)
...eio/tests/readers/extra_readers/test_lif_reader.py 99.15% <ø> (ø)
...eio/tests/readers/extra_readers/test_nd2_reader.py 95.00% <ø> (ø)
...eaders/extra_readers/test_ome_tiled_tiff_reader.py 100.00% <ø> (ø)
...icsimageio/tests/readers/test_array_like_reader.py 0.00% <ø> (-100.00%) ⬇️
aicsimageio/tests/readers/test_glob_reader.py 0.00% <ø> (-100.00%) ⬇️
aicsimageio/tests/readers/test_ome_tiff_reader.py 0.00% <ø> (-95.46%) ⬇️
... and 10 more

... and 12 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@SeanLeRoy SeanLeRoy self-assigned this Mar 16, 2023
SeanLeRoy and others added 2 commits March 20, 2023 16:50
* Resolves gh-434. Default 6 dimension images to TCZYXS order when array-like

* New test for dimension ordering; minor adjustments to existing tests to fix
@SeanLeRoy
Copy link
Collaborator Author

SeanLeRoy commented Mar 20, 2023

The actual test fixes branch has been merged into this one + a new commit which removes a test that was previously evergreen and resulted in a boto3 credentials error. Instead of making it work by setting up the credentials or whatever else was necessary I removed that test completely since without being REMOTE it was the same setup as another test in the same set besides an expectation to raise IndexError since both tests couldn't be true at once I removed the IndexError test as a duplicate.

I still expect a few tests to fail due to a (unrelated to this PR) tifffile error which I have a PR coming soon for.

Copy link
Collaborator

@evamaxfield evamaxfield left a 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 for your work on this -- merge whenever

@SeanLeRoy SeanLeRoy merged commit eb42918 into main Mar 21, 2023
@SeanLeRoy SeanLeRoy deleted the bugfix/fix-tests-not-asserting-error-raised branch March 21, 2023 16:26
@AetherUnbound
Copy link
Collaborator

I know I'm a little late with this one, but per pytest's documentation:

Using pytest.raises() is likely to be better for cases where you are testing exceptions your own code is deliberately raising, whereas using @pytest.mark.xfail with a check function is probably better for something like documenting unfixed bugs (where the test describes what “should” happen) or bugs in dependencies.

It seems like our case points more towards the former which would imply we should be using pytest.raises instead (and pytest.raises should be raising an exception if the expected exception doesn't occur). I'd be really interested in finding more information about which test was passing when it should not have been!

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.

5 participants