Tests/bugfix: clean up build stage handling; no user config.yaml change (#12651, #12798, #13009) #12857

tldahlgren · 2019-09-18T02:36:47Z

This PR restructures tests needing to temporarily change theconfig:build_stage path such that the tests no longer create or overwrite the setting in $HOME/.spack/config.yaml. The change also appears to resolve stage path related issues when filtering tests on staging.

This PR consists of two distinct commits. The first provides support for nested overrides scopes, which are created via the spack.config.override context manager. The second commit changes how the autouse'd mock_stage and assorted test fixtures and tests establish their temporary stage directories.

TODO :

tldahlgren · 2019-09-19T01:33:21Z

@tgamblin ping

tldahlgren · 2019-09-19T18:03:26Z

FYI. I just squashed the last 5 commits so this PR covers two commits:

Add support for nested "overrides" scopes. (5f72fe9)
tests: cleanup config:build_stage handling (fixes tests: test_stage fails when run with -k #12651, "spack test" wrongly adds directory to config.yaml #12798). (09d53a4)

tldahlgren · 2019-09-19T19:28:11Z

@chissg Does this resolve your $HOME/.spack/config.yaml issue?

greenc-FNAL · 2019-09-19T19:38:16Z

@chissg Does this resolve your $HOME/.spack/config.yaml issue?

@tldahlgren Not sure I'd know until I interrupt a testing run in just the right (wrong) way and don't get the problem. I'll let you know if the dog doesn't bark!

tldahlgren · 2019-09-19T19:54:24Z

@chissg Does this resolve your $HOME/.spack/config.yaml issue?

@tldahlgren Not sure I'd know until I interrupt a testing run in just the right (wrong) way and don't get the problem. I'll let you know if the dog doesn't bark!

Np.

tldahlgren · 2019-09-20T00:15:52Z

FYI. The $HOME/.spack/config.yaml file is only present for me when I run the tests. So it's absence after testing -- assuming I made sure to remove it before beginning the tests -- was how I confirmed this PR worked.

becker33

Some questions, nothing that I'm confident needs to change but I wanted to ask a couple questions

I think some of these might be things that you've written exactly how they need to be written, and we'll just need to add comments for maintainability.

lib/spack/spack/config.py

lib/spack/spack/test/stage.py

becker33 · 2019-09-20T22:10:54Z

lib/spack/spack/test/stage.py

-    spack.config.set('config', {'build_stage': [str(tmpdir)]}, scope='user')
-    yield tmpdir
-    spack.config.set('config', {'build_stage': current}, scope='user')
+    shutil.rmtree(base)


What if someone already happens to have a file in $spack named test-stage?

Fair enough.

How about I make it a hidden file? Or I could sequence it.

Of the two options I mentioned, which would you prefer? Or do you have an alternate?

FYI. Changed to hidden directory.

That at least makes it more likely it's safe. We should probably put spack in the name to be a little safer still

lib/spack/spack/test/stage.py

scheibelp

I have a few preliminary questions/comments.

lib/spack/spack/test/config.py

lib/spack/spack/test/stage.py

scheibelp · 2019-09-23T20:15:18Z

lib/spack/spack/test/conftest.py

-        # Ensure the source directory exists
-        source_path = new_stage.join(spack.stage._source_path_subdir)
-        source_path.ensure(dir=True)
+        monkeypatch.setattr(spack.stage, '_stage_root', new_stage_path)


(question) Is the nomockstage argument still required with this method of adjusting the stage path?

The mock_stage fixture creates (during setup) and removes when present (during teardown) of the "well known" source path within the stage root.

Given mock_stage is (now at) the function level and autouse, this file system activity is performed whether the test case needs it or not. So I would think it would be better to either revisit having it autouse (which should be mirrored in the use of the check_for_leftover_stage_files fixture) OR make more use of the nomockstage mark to avoid the unnecessary overhead.

BTW I meant "revisit" in a separate PR.

So I would think it would be better to either revisit having it autouse (which should be mirrored in the use of the check_for_leftover_stage_files fixture) OR make more use of the nomockstage mark to avoid the unnecessary overhead.

I prefer the former and agree that if it comes to this it ought to be dealt with in a later PR. However, I'm curious about:

Given mock_stage is (now at) the function level and autouse, this file system activity is performed whether the test case needs it or not.

I was wondering if there might be a way to make the tests work regardless of whether this runs. The goal of this is to ensure that for the purposes of testing that the stages are created in a test-managed directory. I'm wondering if introducing an additional root path component would reconcile the concerns of the tests: e.g. right now the stage path read from config.yaml is relative to / but we could add a module-level variable to Stage that adjusts this root to be a temporary directory.

Does that seem reasonable? It could also be moved to another refactor PR but I prefer it to the approach of removing autouse or using nomockstage. I think it could be beneficial even in the case of removing the autouse but IMO it would also make it so autouse was only a performance problem.

I wasn't the one to add nomockstage to address unit testing issues. (Not sure I can reproduce their problem.)

I'll have to give this more thought.

Created #13065

OK that makes sense for this one, I pulled the PR and for the other tests I tried with/without nomockstage and it doesn't appear to be needed elsewhere.

lib/spack/spack/test/stage.py

tldahlgren · 2019-09-25T22:11:38Z

FYI. Once again narrowed this PR to two commits: one for nested overrides and the other for the tests.

tldahlgren · 2019-09-27T16:22:21Z

@becker33 @scheibelp ping

adamjstewart · 2019-10-02T16:34:06Z

Can confirm that this fixes #13009

tldahlgren · 2019-10-07T22:27:46Z

@becker33 @scheibelp ping

Note there will likely be material conflicts requiring manual resolution between this and #12733 .

scheibelp

I think although there is at least one necessary use of nomockstage, the other uses don't appear to be required. I pointed this out for most tests. Beyond that I have a couple other minor requests.

lib/spack/spack/test/stage.py

lib/spack/spack/test/config.py

scheibelp · 2019-10-08T22:45:06Z

lib/spack/spack/test/stage.py

-        """Ensure an instance path stage root is a suitable path."""
-        assert spack.stage._stage_root is None
+                # Make sure cached stage path value was changed appropriately
+                assert spack.stage._stage_root == path


I think assert path == test_path or assert spack.stage._stage_root == test_path would be more-straightforward. As-is this is testing that spack.stage._stage_root == spack.stage.get_stage_root() (which IMO is less important to test but if you disagree then I'd prefer assert spack.stage._stage_root == spack.stage.get_stage_root()).

Good point. It looks like, from the diffs, this was an existing check that got "moved". I like the second option over the last one since path is being used in another assertion at line 750.

scheibelp · 2019-10-08T22:53:01Z

lib/spack/spack/test/conftest.py

-        # Ensure the source directory exists
-        source_path = new_stage.join(spack.stage._source_path_subdir)
-        source_path.ensure(dir=True)
+        monkeypatch.setattr(spack.stage, '_stage_root', new_stage_path)


OK that makes sense for this one, I pulled the PR and for the other tests I tried with/without nomockstage and it doesn't appear to be needed elsewhere.

scheibelp

The tmp_build_stage_dir fixture does not appear to be required. I think it adds confusion since it modifies the config so I think it would be good to remove it here. I have added comments to the two tests that use it and how to remove it from them.

scheibelp · 2019-10-09T21:37:30Z

lib/spack/spack/test/stage.py

-    Note that it can be important for other tests that the previous settings be
-    restored when the test case is over.
-    """
+def mock_stage_archive(tmp_build_stage_dir):


If I set test_stage_path = spack.stage.get_stage_root() then this doesn't seem to require the tmp_build_stage_dir fixture (but it will in that case need the tmpdir fixture for creating the archive later).

Since every test uses mock_stage, this will be fine, and in fact this is kind of a strange piece of data to keep track of (although not from your work but rather a legacy behavior) since it's just used to verify that once the archive is staged, that the stage is created where you expect. Overall I figure this field ought to be removed from the Archive object this creates but that doesn't need to happen here.

FWIW. This fixture is mock_stage_archive, which is currently used by 14 stage tests, not mock_stage. It is used to create an archive file for staging.

The mock_stage fixture currently ensures the presence of the well-known source path for staging tests and removes the created path if it still exists.

As I revisit the two fixtures, I think I can see your concern but would still prefer to defer this matter to a separate PR due to other priorities.

Added to #13065

scheibelp · 2019-10-09T21:51:13Z

lib/spack/spack/test/stage.py

-
-    spack.config.set('config', {'build_stage': current}, scope='user')
-
-
 def test_stage_create_replace_path(tmp_build_stage_dir):
    """Ensure stage creation replaces a non-directory path."""
    _, test_stage_path = tmp_build_stage_dir


This instance of tmp_build_stage_dir can also be removed and replaced with test_stage_path = spack.stage.get_stage_root(), although you have to call stage.destroy at the end of the test (and I recommend putting that in a try/finally block to avoid impacts to other tests.

Also relevant to addition of this issue in #13065

lib/spack/spack/test/stage.py

scheibelp · 2019-10-09T22:10:25Z

lib/spack/spack/test/stage.py

-            assert spack.stage._stage_root is None
+        with spack.config.override('config:build_stage', stage_path):
+            stage_root = spack.stage.get_stage_root()
+            assert stage_path == stage_root


I think test_get_stage_root_in_spack is already testing this (i.e. that the override is working properly) so I don't think that needs to be checked in this test.

What's the harm in confirming it is as expected before performing purge?

Each test should focus on testing one thing (or as little as possible). Someone reading this test could get confused about how the assert confirms the behavior the test is focused on.

Do you have a reference for this statement?

Testing is very dependent on the structure/modularity of the code being tested.

Test checks depend on the nature (and granularity) of the test. For example, unit tests of functions/routines/methods should be limited to the associated side-effect(s), for example. (Ideally you'd have 'all' "happy path" and "unhappy" path tests for every function/routine/method covered.)

Tests that build up functionality may need more checks.

In this particular case, it combined get_stage_root and the stage purge functions. I am not comfortably with the idea of not ensuring that get_stage_root is returning the expected path before calling purge not because of the existing functionality but as "protection" against future changes.

tldahlgren · 2019-10-09T22:25:01Z

The tmp_build_stage_dir fixture does not appear to be required. I think it adds confusion since it modifies the config so I think it would be good to remove it here. I have added comments to the two tests that use it and how to remove it from them.

Testing modification of the configuration is the point of the fixture, which also does cleanup. The fixture is used by test_stage_create_replace_path and the mock_stage_archive fixture (the latter used for 14 tests).

Changing it as you suggest is not exactly trivial given the trade-offs.

Can't this work be deferred to a separate issue?

tldahlgren · 2019-10-10T00:01:13Z

@becker33 @scheibelp ping

Note that I question some of the changes wrt their inclusion in this PR and have not yet reduced this back down to the two main commits.

…12798)

Add support for nested "overrides" scopes.

5f72fe9

tldahlgren added WIP tests General test capability(ies) stage labels Sep 18, 2019

tldahlgren self-assigned this Sep 18, 2019

tldahlgren changed the title ~~[WIP] Tests/cleanup build stage~~ [WIP] Tests: cleanup build stage handling Sep 19, 2019

tldahlgren added the refactoring label Sep 19, 2019

tldahlgren changed the title ~~[WIP] Tests: cleanup build stage handling~~ Tests: cleanup build stage handling Sep 19, 2019

tldahlgren removed the WIP label Sep 19, 2019

tldahlgren force-pushed the tests/cleanup-build-stage branch from a337753 to 09d53a4 Compare September 19, 2019 18:00

tldahlgren added the impact-high label Sep 19, 2019

tldahlgren requested review from tgamblin and scheibelp September 19, 2019 19:12

tldahlgren changed the title ~~Tests: cleanup build stage handling~~ Tests: clean up build stage handling Sep 19, 2019

tldahlgren added the bugfix Something wasn't working, here's a fix label Sep 19, 2019

tldahlgren requested a review from alalazo September 19, 2019 22:58

becker33 reviewed Sep 20, 2019

View reviewed changes

scheibelp reviewed Sep 23, 2019

View reviewed changes

tldahlgren force-pushed the tests/cleanup-build-stage branch from 8d29a2f to ef4b266 Compare September 25, 2019 22:10

tldahlgren added impact-medium and removed impact-high labels Sep 27, 2019

tldahlgren force-pushed the tests/cleanup-build-stage branch from cdd9023 to 67cf1ae Compare September 30, 2019 21:06

tldahlgren force-pushed the tests/cleanup-build-stage branch 3 times, most recently from 6d00f5e to 12073ba Compare October 1, 2019 00:25

tldahlgren mentioned this pull request Oct 2, 2019

spack test leaves behind config.yaml file #13009

Closed

tldahlgren mentioned this pull request Oct 7, 2019

tests: revisit mock_stage, nomockstage, and tmp_build_stage_dir #13065

Open

tgamblin added this to Todo in Spack v0.13.0 release Oct 8, 2019

tldahlgren moved this from Todo to In Review in Spack v0.13.0 release Oct 8, 2019

scheibelp requested changes Oct 8, 2019

View reviewed changes

tldahlgren changed the title ~~Tests: clean up build stage handling~~ Tests/bugfix: clean up build stage handling Oct 9, 2019

scheibelp requested changes Oct 9, 2019

View reviewed changes

Spack v0.13.0 release automation moved this from In Review to In Progress Oct 9, 2019

scheibelp reviewed Oct 9, 2019

View reviewed changes

lib/spack/spack/test/stage.py Outdated Show resolved Hide resolved

scheibelp reviewed Oct 9, 2019

View reviewed changes

tests: cleanup config:build_stage handling (fixes spack#12651, spack#…

9ab4b47

…12798)

tldahlgren force-pushed the tests/cleanup-build-stage branch from 656f538 to 9ab4b47 Compare October 10, 2019 18:53

scheibelp approved these changes Oct 10, 2019

View reviewed changes

Spack v0.13.0 release automation moved this from In Progress to In Review Oct 10, 2019

tldahlgren merged commit 93a44c8 into spack:develop Oct 10, 2019

Spack v0.13.0 release automation moved this from In Review to Done Oct 10, 2019

tldahlgren deleted the tests/cleanup-build-stage branch October 15, 2019 20:55

tldahlgren changed the title ~~Tests/bugfix: clean up build stage handling~~ Tests/bugfix: clean up build stage handling; no user config.yaml (#12651, #12798, #13009) Oct 16, 2019

tldahlgren changed the title ~~Tests/bugfix: clean up build stage handling; no user config.yaml (#12651, #12798, #13009)~~ Tests/bugfix: clean up build stage handling; no user config.yaml change (#12651, #12798, #13009) Oct 16, 2019

Tests/bugfix: clean up build stage handling; no user config.yaml change (#12651, #12798, #13009) #12857

Tests/bugfix: clean up build stage handling; no user config.yaml change (#12651, #12798, #13009) #12857

Conversation

tldahlgren commented Sep 18, 2019 • edited

tldahlgren commented Sep 19, 2019

tldahlgren commented Sep 19, 2019

tldahlgren commented Sep 19, 2019

greenc-FNAL commented Sep 19, 2019

tldahlgren commented Sep 19, 2019

tldahlgren commented Sep 20, 2019

becker33 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scheibelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tldahlgren commented Sep 25, 2019

tldahlgren commented Sep 27, 2019

adamjstewart commented Oct 2, 2019

tldahlgren commented Oct 7, 2019

scheibelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scheibelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tldahlgren Oct 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tldahlgren commented Oct 9, 2019

tldahlgren commented Oct 10, 2019 • edited

tldahlgren commented Sep 18, 2019 •

edited

tldahlgren Oct 10, 2019 •

edited

tldahlgren commented Oct 10, 2019 •

edited