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

DM-41750: Fix some problems with global state in tests #908

Merged
merged 11 commits into from Nov 22, 2023
Merged

Conversation

timj
Copy link
Member

@timj timj commented Nov 17, 2023

Sometimes tests would fail depending on what other tests have already run.

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (8ca025c) 87.45% compared to head (869a33c) 87.30%.

Files Patch % Lines
python/lsst/daf/butler/_storage_class.py 41.66% 4 Missing and 3 partials ⚠️
...hon/lsst/daf/butler/datastores/chainedDatastore.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #908      +/-   ##
==========================================
- Coverage   87.45%   87.30%   -0.15%     
==========================================
  Files         286      286              
  Lines       37144    37162      +18     
  Branches     7828     7833       +5     
==========================================
- Hits        32485    32446      -39     
- Misses       3474     3519      +45     
- Partials     1185     1197      +12     

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

@timj timj marked this pull request as ready for review November 21, 2023 20:26
Copy link
Contributor

@andy-slac andy-slac 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, one minor comment.

This prevents problems if other tests have reset the lsst logger
level since it forces the lsst logger into the right state
for log capture.
Some other test may have configured a global log handler to
emit logs to stdout instead of stderr
Python 3.12 insists on using these so the parser has to also
be updated to strip the +00:00 before astropy parses it.
3.12 wants to use datetime.UTC but that is not part of 3.10.
The metrics test repo code was creating storage classes with the same
names as those defined in the test storage class YAML. Unfortunately these
definitions were slightly different and test ordering could affect which
definition was used. Furthermore, these manually created storage classes
were not created as special subclasses of StorageClass (unlike those
from configs) and so inheritFrom specifications in the YAML file
were not working properly since they all ended up inheriting from
the StorageClass base class and losing properties. Ideally the
test repo code would use the YAML definitions directly.
The S3 tests were all sharing the same temp directory
previously and since files were not being cleaned up
the test order changing could make some tests fail.
@timj timj merged commit 593d116 into main Nov 22, 2023
16 of 17 checks passed
@timj timj deleted the tickets/DM-41750 branch November 22, 2023 15:41
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