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

Pin pytest-xdist to <3.5 #3274

Merged
merged 2 commits into from Nov 29, 2023

Conversation

ericpre
Copy link
Member

@ericpre ericpre commented Nov 29, 2023

Pin pytest-xdist to <3.5 - issue seem to have introduced in pytest-dev/pytest-xdist#778, even if at first this appear to be unrelated (we use loadfile and not loadscope)

…v/pytest-xdist#778, even if at first this appear to be unrelated (we use `loadfile` and not `loadscope`)
@ericpre ericpre mentioned this pull request Nov 29, 2023
6 tasks
Copy link

codecov bot commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2c84104) 80.85% compared to head (0d2f0f0) 80.78%.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           RELEASE_next_major    #3274      +/-   ##
======================================================
- Coverage               80.85%   80.78%   -0.07%     
======================================================
  Files                     140      140              
  Lines                   20400    20400              
  Branches                 4826     4826              
======================================================
- Hits                    16494    16481      -13     
- Misses                   2829     2838       +9     
- Partials                 1077     1081       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@CSSFrancis
Copy link
Member

@ericpre This does seem to fix the problem but I'm curious as to why this is failing. It seems like the linked PR reordered the way in which pytest-xdist runs (prioritizing things with the largest scope and working backwards). It seems like this error is more than likely a result of a saved state on our end that wasn't being caught simply because of the order in which things were run rather than a bug related to ``pytest-xdist` or am I not understanding this?

@ericpre
Copy link
Member Author

ericpre commented Nov 29, 2023

I don't know, and I am a bit surprised that pytest-dev/pytest-xdist#778 is related to it because we don't use this scheduler...
The reason to to use loadfile to our end was to avoid issue with reading files from different tests, which was problematic with the IO code. This can possibly be revisited now that the IO code move to rosettasciio but at the same time, I think that pinning pytext-xdist is not a big deal and we can come back to it later!

The issue is only for image comparison of plot testing because the wrong matplotlib colormap is used - not the one that is expected to be used, so this is indeed something playing up with the setting of the matplotlib colormap.

@CSSFrancis
Copy link
Member

CSSFrancis commented Nov 29, 2023

Could it be related to the setup_teardown function here not being properly run in some situations? I'm not sure if the scope parameter is actually being used currently.

@pytest.fixture
def setup_teardown(request, scope="class"):
try:

@CSSFrancis
Copy link
Member

@ericpre For the sake of fixing this and moving forward I'm okay with pinning this to <3.5 but we should probably make an issue to track this.

There does seem to be something weird related to the test file there...

@ericpre
Copy link
Member Author

ericpre commented Nov 29, 2023

Done: #3275

@CSSFrancis
Copy link
Member

I can merge this and rebase #3270

@CSSFrancis CSSFrancis merged commit f75a8d7 into hyperspy:RELEASE_next_major Nov 29, 2023
22 of 23 checks passed
@ericpre
Copy link
Member Author

ericpre commented Nov 29, 2023

Yes plesse!

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.

None yet

2 participants