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-11514: Fix a race condition in tests under pytest-xdist #273
Conversation
78cf4c1
to
5b8d9ea
Compare
@PaulPrice can you take a look at this for me please? Should be fairly non-controversial. |
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 implementation suggestions.
tests/test_apCorrMap.py
Outdated
@@ -109,7 +109,7 @@ def testPersistence(self): | |||
|
|||
def testExposurePersistence(self): | |||
"""Test that the ApCorrMap is saved with an Exposure""" | |||
filename = "testApCorrMap.fits" | |||
filename = "testApCorrMap-exposure.fits" |
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.
I suggest using:
with lsst.utils.tests.getTempFilePath(".fits") as filename:
That gets you automatic cleanup, and is more immune to cargo culting because it chooses a name based on the test.
Ditto in testExposureRecordPersistence
.
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.
good idea.
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.
Implemented temp files.
tests/test_sourceMatch.py
Outdated
@@ -181,6 +181,7 @@ def testPhotometricCalib(self): | |||
s.setDec(dec * afwGeom.degrees) | |||
s.set(self.table.getPsfFluxKey(), psfMags[band]) | |||
|
|||
ifd.close() |
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.
Is it worth using the context manager? This is exactly what it's for.
with open(os.path.join(afwdataDir, "CFHT", "D2", "sdss.dat"), "r") as ifd:
allLines = ifd.readlines()
....
for line in allLines:
Ditto below.
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.
I'm happy to do that. The little Robert voice in my head was wondering if the big indenting diff was worth it.
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.
He's in my head too, which is why it was just a suggestion.
I slightly lean towards using the context manager, but it's up to you.
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.
I made that change.
pytest can result in tests from the same class running in parallel, leading to a race where one test writes the file and the other test removes it before it can be read. The simplest solution is to use distinct file names for each of the tests.
The latter is deprecated in python3
The latter is deprecated in python3
Failing to do this results in a resource warning.
This fixes a resource warning.
785f983
to
1108928
Compare
Without a third argument the line number reported by the deprecation warnings is the line number of the warning itself. This is not useful to a developer trying to determine which method was deprecated. This changes the situation so that the caller line number is reported by setting stacklevel=2.
1108928
to
1e68ab8
Compare
Also fix some deprecation warnings and use of AFW_DIR.