Skip to content

TEST: Use class variables and close open file handles in ECAT tests #1155

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

Merged

Conversation

DimitriPapadopoulos
Copy link
Contributor

ResourceWarning: unclosed file <_io.BufferedReader name='/home/runner/work/nibabel/nibabel/virtenv/lib/python3.9/site-packages/nibabel/tests/data/tinypet.v'>

@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Base: 92.38% // Head: 92.38% // No change to project coverage 👍

Coverage data is based on head (a33a2af) compared to base (d0e6363).
Patch has no changes to coverable lines.

❗ Current head a33a2af differs from pull request most recent head f0770b9. Consider uploading reports for the commit f0770b9 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1155   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files          98       98           
  Lines       12257    12257           
  Branches     2520     2520           
=======================================
  Hits        11324    11324           
  Misses        612      612           
  Partials      321      321           

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as draft December 13, 2022 03:51
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the ResourceWarning_unclosed_file branch from 63cb431 to 1253e2c Compare December 13, 2022 03:52
@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review December 13, 2022 03:59
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the ResourceWarning_unclosed_file branch 2 times, most recently from 58f7b59 to fb3bb8b Compare December 13, 2022 06:29
@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as draft December 13, 2022 06:33
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the ResourceWarning_unclosed_file branch from fb3bb8b to 3e25a5d Compare December 13, 2022 06:38
@effigies
Copy link
Member

Tests should be fixed if you rebase on master...

@DimitriPapadopoulos DimitriPapadopoulos force-pushed the ResourceWarning_unclosed_file branch 3 times, most recently from e2033b3 to 07e25d0 Compare December 18, 2022 08:29
@DimitriPapadopoulos
Copy link
Contributor Author

I am still getting errors, they seem related to CI instability.

@effigies
Copy link
Member

Hmm. Looks like there might be a between-test dependency that can get broken by pytest-xdist. I ran several times and thought I would have caught a non-determinism...

@effigies
Copy link
Member

Okay, let's try again...

ResourceWarning: unclosed file <_io.BufferedReader name='/home/runner/work/nibabel/nibabel/virtenv/lib/python3.9/site-packages/nibabel/tests/data/tinypet.v'>
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the ResourceWarning_unclosed_file branch from a33a2af to f0770b9 Compare January 1, 2023 15:27
@effigies
Copy link
Member

effigies commented Jan 2, 2023

This doesn't seem to have eliminated the warning.

@effigies effigies added this to the 5.0.0 milestone Jan 2, 2023
@DimitriPapadopoulos
Copy link
Contributor Author

Removing all warnings requires a rewrite of the testing classes.

In any case, this does the right thing by changing ecat_fileself.example_file.

@effigies effigies changed the title MNT: Fix CI warnings TEST: Use class variables and close open file handles in ECAT tests Jan 2, 2023
@effigies
Copy link
Member

effigies commented Jan 2, 2023

Yeah, I think we need to move to a different fixture approach to ensure better handling of resources. Are you good to merge this as-is, or are there things you still want to try to do here?

@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review January 2, 2023 17:22
@DimitriPapadopoulos
Copy link
Contributor Author

I'm good, I can rewrite the commit message if needed.

@effigies effigies merged commit 283bd8e into nipy:master Jan 2, 2023
@DimitriPapadopoulos DimitriPapadopoulos deleted the ResourceWarning_unclosed_file branch January 2, 2023 19:58
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.

2 participants