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

Directories created by scripts_regression_tests should have unique names #401

Closed
billsacks opened this issue Aug 16, 2016 · 7 comments
Closed
Assignees

Comments

@billsacks
Copy link
Member

I noticed that I was getting inconsistent results in my runs of scripts_regression_tests. I specifically noticed this in test_createnewcase (__main__.B_TestCreateNewcase), which seemed to pass sometimes and fail sometimes. I began to suspect (but have not confirmed) that this was because I was running scripts_regression_tests out of two different cime clones at once (I was running this for my own branch at the same time as I was running it for master).

In looking a little closer, I found that, at least for this test, a directory is created with a hard-coded path, meaning that tests can stomp on each other. I'm guessing that was what happened in my case... but even if not, this opens the door to problems running multiple instances of the test suite at once, so it seems this should be fixed:

    def setUp(self):
        self._testroot = MACHINE.get_value("CESMSCRATCHROOT")

    def test_createnewcase(self):
        testdir = os.path.join(self._testroot, 'scripts_regression_tests.testcreatenewcase')
        if os.path.exists(testdir):
            shutil.rmtree(testdir)

Would it be possible to create a directory with a unique ID in this case? Using https://docs.python.org/2/library/tempfile.html seems like it could be ideal, but maybe not necessary if there's some reason to do something different.

@jedwards4b
Copy link
Contributor

I did try to do it as you suggest with tempfile but ran into several problems - one I remember is that it kept trying to put things into the /tmp directory which is generally not a shared filesystem. You are welcome to take another stab at it.

@billsacks
Copy link
Member Author

@jedwards4b : When you say that /tmp is not a shared filesystem, is this referring to its not being shared between (e.g.) login nodes and compute nodes? I ask because I have introduced the use of tempfile in some of my new unit tests (seeing that it's done for some existing tests)... I'm thinking this is okay in this case, because my new unit tests aren't doing any job submissions, but please let me know if I'm mistaken.

@jedwards4b
Copy link
Contributor

RIght /tmp is not shared between login nodes and compute nodes or between compute nodes.
It's also quite small in some cases and depending on the test you could easily run out of disk space.
The testcreatenewcase does not submit anything either - but I was running into problems using tempfile. I don't recall now what it was - I think I wanted a partial path and could only figure out how to get a full path out of it. Also scripts_regression_tests will remove directories after tests have successfully completed and leave only directories associated with failed tests behind.

@billsacks
Copy link
Member Author

RIght /tmp is not shared between login nodes and compute nodes or between compute nodes.

It's also quite small in some cases and depending on the test you could easily run out of disk space.

Okay, thanks, I think that should still work fine for my unit tests, which generally only create a few bytes of output.

I think I wanted a partial path and could only figure out how to get a full path out of it.

Would this do what you want?:

self._testroot = tempfile.mkdtemp(dir=MACHINE.get_value("CESMSCRATCHROOT"))

(that will create a unique temporary directory under CESMSCRATCHROOT).

@jedwards4b
Copy link
Contributor

I think that I tried it, you are welcome to try again.

@rljacob rljacob added the ready label Aug 16, 2016
@quantheory
Copy link
Contributor

Sounds like the /tmp question was resolved, but I just wanted to mention that /tmp is often placed in RAM, in which case it is not shared between nodes and also will not persist through a system reboot. For this reason it's usually better to use it just for unit tests, rather than full system tests.

@jgfouca
Copy link
Contributor

jgfouca commented Aug 16, 2016

We could always just add a timestamp to the directory.

@jedwards4b jedwards4b self-assigned this Aug 16, 2016
jgfouca added a commit that referenced this issue Aug 16, 2016
…ression_tests

Batch fix reorder scripts regression tests



Reorder tests in scripts_regression_tests (fast first slower later)
Test suite: scripts regression tests
Test baseline:
Test namelist changes:
Test status: 1 known failure in TestTestScheduler
Fixes #401

User interface changes?:

Code review:
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

No branches or pull requests

5 participants