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-11607: Use mkdtemp in tests #61

Merged
merged 1 commit into from Aug 19, 2017
Merged

DM-11607: Use mkdtemp in tests #61

merged 1 commit into from Aug 19, 2017

Conversation

timj
Copy link
Member

@timj timj commented Aug 18, 2017

This fixes the problem with races in pytest xdist.

@timj timj requested a review from n8pease August 18, 2017 16:54
import shutil

testDir = os.path.relpath(os.path.join(getPackageDir('obs_base'), 'tests'))
testDir = os.path.abspath(os.path.dirname(__file__))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We have called this ROOT in most other places. It would help to change that here so as not to confuse it with self.testDir, which is a temporary directory below this.

packageDir = getPackageDir('obs_base')
self.testData = os.path.join(packageDir, 'tests', 'genericAssembler')
self.tearDown()
self.testData = tempfile.mkdtemp(dir=ROOT, prefix='genericAssembler-')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be more consistent with other tests to use "TestCompositeTestCase" (the test class name) as the prefix.

@@ -379,8 +375,7 @@ class TestSubset(unittest.TestCase):
"""A test case for composite object subset keyword."""

def setUp(self):
packageDir = getPackageDir('obs_base')
self.testData = os.path.join(packageDir, 'tests', 'compositeSubset')
self.testData = tempfile.mkdtemp(dir=ROOT, prefix='compositeSubset-')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefix -> "TestSubset"

@@ -446,8 +441,7 @@ class TestInputOnly(unittest.TestCase):
"""A test case for composite input keyword."""

def setUp(self):
packageDir = getPackageDir('obs_base')
self.testData = os.path.join(packageDir, 'tests', 'composite')
self.testData = tempfile.mkdtemp(dir=ROOT, prefix='composite-')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefix -> "TestInputOnly"


testDir = os.path.relpath(os.path.join(getPackageDir('obs_base'), 'tests'))
testDir = os.path.abspath(os.path.dirname(__file__))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testDir -> ROOT

packageDir = getPackageDir('obs_base')
self.testDir = os.path.join(packageDir, 'tests', 'findParentMapper')

self.testDir = tempfile.mkdtemp(dir=ROOT, prefix='findParentMapper-')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefix -> "TestFindParentMapperV1Butler"

dprefix = dprefix + '-'
tempDir = tempfile.mkdtemp(dir=ROOT, prefix=dprefix)
self.tempDirs[prefix] = tempDir
return tempDir
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels complex to me; I think I would have created a top-level temp folder and called that self.testDir, and put the output roots for each test method into that folder. But this seems to work so I'm ok if you want to leave it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems clearer to me so I would like to keep it. I didn't want to create a bunch of temp directories that were not being used so I switched to a test only creating a temp directory when it really needed one. I think the real solution would be to add something to lsst.utils.tests to create a temp directory in a context manager.

Tests can be run in parallel from within a single test case, so temporary
directories can not use a fixed name. Clean up to use proper temp
directories. During this cleanup, getPackageDir is no longer used
to be consistent within tests.
@timj
Copy link
Member Author

timj commented Aug 18, 2017

I've made the changes requested. I'll do a Jenkins run to test.

@timj timj merged commit 4f9f5bb into master Aug 19, 2017
@ktlim ktlim deleted the tickets/DM-11607 branch August 25, 2018 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants