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-28042: configure test temporary directory location via envvar #452

Merged
merged 2 commits into from Dec 17, 2020

Conversation

TallJimbo
Copy link
Member

No description provided.

@n8pease
Copy link
Contributor

n8pease commented Dec 16, 2020

Looks good overall. The one thing that I noticed is that base = os.environ.get("LSST_DAF_BUTLER_TEST_TMP", TESTDIR) is repeated often enough, I wish there was a good way to reuse that, but it seems to appear in enough different places (setUp, setUpClass) that it's not a slam-dunk to use a common base class, and putting it in a helper function & importing that from e.g. lsst.daf.butler.tests results in more lines of code, not less, though the helper function may still be more reusable & refactorable in the long run.

@timj
Copy link
Member

timj commented Dec 16, 2020

@n8pease I was about to say the same thing. There are many cases where we seem to have the same two lines repeated over and over. Also, the other butler env vars start with DAF_BUTLER_ and not LSST_DAF_BUTLER_...

@TallJimbo
Copy link
Member Author

Happy to switch to DAF_BUTLER_....

Not sure I can get the number of repeated lines down to one instead of two, but I'll give it a shot; I should be able to at least make it a different two lines so the number of times the envvar name appears in a string is drastically reduced.

@TallJimbo
Copy link
Member Author

Ok, completely refactored version is up (just amended the last commit). All of the logic is now in some functions in daf.butler.tests.utils, including the shutils bit that we use to remove temporary directories. The envvar name (now DAF_BUTLER_TEST_TMP) appears in just two places - one of those utility functions, and the SConstruct code that propagates it.

This also gathers all of our temporary directory creation and removal
code into a couple of test utility functions.
@TallJimbo TallJimbo merged commit a584ca4 into master Dec 17, 2020
@TallJimbo TallJimbo deleted the tickets/DM-28042 branch December 17, 2020 03:36
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

3 participants