Add test deptype by scheibelp · Pull Request #5132 · spack/spack

scheibelp · 2017-08-16T20:52:04Z

Fixes #1279

Specify that certain dependencies are for testing only. These packages are treated like build dependencies:

They don't affect the package hash (unless they influence the concretization of transitive dependencies). One consequence is that if you build a package without requesting tests, if you try again with tests it will not actually perform the tests (since the package is already built and the tests occur as part of the build)
They perform the same environment modifications as build dependencies (during the build of the package)

This also replaces the --run-tests option for spack install with two options

--test-all: same behavior as previous --run-tests option; tests all packages in the DAG
--test-root: test only the top-level package in the dag. I found this useful for the case of libpsl which wanted to run tests for the python dependency in the --test-all case.

I've only tested this so far with libpsl

…ntioned by the user with the 'spack install' command

… test a package

davydden · 2017-08-16T21:07:54Z

Nice! I also like the '--test-root' option.

scheibelp · 2017-08-18T02:21:51Z

Someone brought up the point that something similar to this could be achieved by adding a "test" variant to packages with additional dependencies and automatically enabling this variant when spack install is called with the --test-all option.

The PR is implemented as it is now because some packages don't change at all when testing is added, even if additional dependencies are required for testing (a variant changes the dag hash so building a package with and without testing would count as different package instances). For packages that change when testing is performed (e.g. the binaries differ) they should include a +test variant. IMO a separate PR could handle the work of automatically enabling +test for packages when e.g. --test-all is specified.

adamjstewart · 2017-08-18T12:28:43Z

They don't affect the package hash (unless they influence the concretization of transitive dependencies).

This is pretty frustrating (the second half of the sentence). Whenever I make modifications to Python packages, I try installing all of them to see if they work. I've noticed that I'll end up with 4 or 5 copies of the same package, same version, same variants, same compilers, but different hash. I believe the reason is that certain packages influence the dependency type of other dependencies (for example, a package that has a build/run dep on setuptools will change the hash of another package that would otherwise have a build dep on setuptools. Is there any way we can solve this? Is the new concretizer actually happening or what?

davydden · 2017-08-18T12:37:10Z

The PR is implemented as it is now because some packages don't change at all when testing is added, even if additional dependencies are required for testing

i think this is the most common case. Otherwise, indeed, it should be a variant which probably does something extra than just running tests.

scheibelp · 2017-08-18T14:31:26Z

I believe the reason is that certain packages influence the dependency type of other dependencies (for example, a package that has a build/run dep on setuptools will change the hash of another package that would otherwise have a build dep on setuptools. Is there any way we can solve this?

#4595 ought to help with that (+ maybe a bit of extra work depending on the particulars of your case). ~~It has a merge conflict right now so I need to fix that up.~~ (EDIT: merged now)

naromero77 · 2017-09-01T19:24:42Z

I did some testing with PR#4907 and it seems to work. (though I may have uncovered a buglet in Spack's ctest interface).

One thing to consider, perhaps not relevant for initial version of test deptype, is that there are scenarios where you will want to test a package after it has already been built and installed. This would be relevant for a cross-compile architectures like BlueGene/Q. You compile on the front-end/login node. Then you have to submit a batch job to run the test on back-end/compute nodes.
@wscullin might have more insight into this.

alalazo · 2017-09-07T05:24:54Z

lib/spack/spack/cmd/install.py

    )
    subparser.add_argument(
-        '--run-tests', action='store_true', dest='run_tests',
+        '--test-all', action='store_true', dest='test_all',


What do you think about having a single argument --test that could take values all and root? This will make the 2 options mutually exclusive, and allow for seamless extensions in the future without adding additional arguments.

I like the idea.

I agree - I'll try to add this today

… option that can be either 'root' or 'all' (or unset)

scheibelp · 2017-09-11T17:27:15Z

Reopening to rerun tests

tgamblin · 2017-09-28T07:00:56Z

Is there any way we can solve this? Is the new concretizer actually happening or what?

@adamjstewart: too slowly but it's happening. I'm hoping it fixes all the pain!

tgamblin · 2017-09-28T07:07:32Z

One thing to consider, perhaps not relevant for initial version of test deptype, is that there are scenarios where you will want to test a package after it has already been built and installed. This would be relevant for a cross-compile architectures like BlueGene/Q. You compile on the front-end/login node. Then you have to submit a batch job to run the test on back-end/compute nodes.

@naromero77: I have kicked around some ideas for this with Barry Smith and I really would really like to support this with a simple command API (e.g., spack test <pkg>). It is tricky because some packages want to run tests in the build directory, and others run tests against an actual install. So there needs to be a way to preserve the build directory for the packages that need it, and that is tricky because Spack builds in $TMP by default, which is typically volatile. Still worse, there are staging issues -- tests may want to run on compute nodes, but we build in $TMP, which is a local filesystem. And even even worse, some packages build lots of things as part of the test suite (PETSc), which is not ideal for x-compiles, as you mention.

So there are things to work out. I think it's doable but there's a larger topic around how best to work all the HPC details in. Everything's easier on plain old intel linux :(.

tgamblin

@scheibelp: some really minor change requests and some comments.

This PR is pretty awesome -- it adds good enough test dependencies and it only adds 3 lines to spec.py. Nice.

I would like to eventually put in better testing capabilities like @naromero77 wants but I think this is pretty good for now, and it greatly enhances what is already there.

Do you want to merge this first, or should we merge #5476 first? I think they will conflict slightly, but rebasing this may be easier. Thoughts?

tgamblin · 2017-09-28T08:43:18Z

lib/spack/spack/cmd/install.py

        help="spec of the package to install"
    )
    subparser.add_argument(
-        '--run-tests', action='store_true',


I think we should probably keep this argument here for a little while -- I suspect people use it in their CI scripts. Can you leave it in like this?

subparser.add_argument( '--run-tests', action='store_const', dest='test', const='all', help='run package tests during installation (same as --test=all)'

I think that will interact correctly with the new --test= (though they should probably be mutually exclusive).

Just throwing in an idea. If we want to be backward compatible, we could write a custom action that handles both --test= and --run-tests. The action should tty.warn users if they are employing --run-tests and suggest an update to the --test=all equivalent. We could leave the warning there for a while and then be "justified" for a later removal of the option.

tgamblin · 2017-09-28T09:04:24Z

lib/spack/spack/test/cmd/install.py

+@pytest.mark.usefixtures('mock_calls_for_install', 'builtin_mock', 'config')
+def test_install_runtests():
+    install('--test=root', 'dttop')
+    assert spack.package_prefs.PackageTesting.test.call_count == 1


Seems like you could check not just the call count but whether the only thing in there is dttop. PackageTesting has the information since test() is passed the package name, right? Checking for 1 call is necessary but not sufficient.

tgamblin · 2017-09-28T09:05:06Z

lib/spack/spack/test/cmd/install.py

+    assert spack.package_prefs.PackageTesting.test.call_count == 1
+
+    install('--test=all', 'a')
+    assert spack.package_prefs.PackageTesting.test_all.call_count == 1


tgamblin · 2017-09-28T09:09:23Z

lib/spack/spack/test/spec_dag.py

    return _mock


+class MockPackage(object):


I can't decide whether I like these or not. They're obviously useful and the implementation here is pretty simple and clean. But we also have builtin_mock -- should these be new packages there? These have the advantage that the info you need to understand a test is entirely contained within the test, while builtin.mock is becoming pretty sprawling. OTOH, builtin.mock actually tests package parsing and the Spack DSL. So I dunno.

Probably it is ok to have both, at least for now. So maybe MockPackage and friends should go in conftest.py so they're usable everywhere?

These have the advantage that the info you need to understand a test is entirely contained within the test, while builtin.mock

That's the main reason I did this and I think it's a really good one. I also use exactly the same new definitions when testing DAGs in #4595 and I think it will be useful for exercising any complex DAG structure.

OTOH, builtin.mock actually tests package parsing and the Spack DSL. So I dunno.

I think that while this advocates for not eliminating builtin.mock, it does not mean this is redundant. Testing package parsing and Spack's DSL is important but doesn't have to be done in every test. In fact, arguably unit tests should be as targeted as possible and IMO more tests would benefit from using the MockPackage objects defined here instead of builtin.mock.

scheibelp · 2017-09-28T23:59:41Z

Thanks for the review! I'll work on addressing the comments by the end of 9/29.

naromero77 · 2017-09-29T00:22:15Z

Many thanks

scheibelp · 2017-09-29T02:24:16Z

Do you want to merge this first, or should we merge #5476 first? I think they will conflict slightly, but rebasing this may be easier. Thoughts?

@tgamblin I think either can go in first. I have a slight preference for this going in first since I think it touches less of the core. I wouldn't hold up one for the other though.

…l, and make it mutually exclusive with the --test option

tgamblin · 2017-09-29T02:54:01Z

@scheibelp: ok sounds good -- let's merge this first. I think the changes here are pretty small, so if @naromero77 wants to test with this and dependency patching as they are now, I think we should merge this, dep patching, and qmcpack once you update this one.

…dd check for --run-tests

… available

scheibelp · 2017-09-29T23:36:09Z

@tgamblin I have made edits to address review comments. Let me know if more is needed.

tgamblin · 2017-09-30T05:08:21Z

@scheibelp: thanks!

tgamblin · 2017-09-30T09:07:43Z

@naromero77: this is merged.

naromero77 · 2017-10-01T03:44:51Z

@tgamblin thanks

scheibelp added 7 commits August 10, 2017 14:10

initial work

b097333

make run_tests configurable on a per-package basis (cheating for now)

c08e481

add --test-root option which performs test for the top-level specs me…

ad83e31

…ntioned by the user with the 'spack install' command

rename '--run-tests' install command line arg to '--test-all'

371e5c1

was checking the child instead of the parent when checking whether to…

3d7d7d2

… test a package

tweak libpsl valgrind dependency

3cd5bac

remove run_tests parameter from Package.do_install

2253d62

scheibelp added the WIP label Aug 16, 2017

adamjstewart added dependencies specs labels Aug 18, 2017

scheibelp added 4 commits August 21, 2017 10:41

merge from develop

d2ee930

add tests

063a5c4

merge from develop

2149318

flake8 fix

7580866

scheibelp mentioned this pull request Aug 28, 2017

qmcpack: new package #4907

Merged

alalazo reviewed Sep 7, 2017

View reviewed changes

scheibelp added 2 commits September 8, 2017 13:18

replace '--test-all' and '--test-root' options with a single '--test'…

7a5a603

… option that can be either 'root' or 'all' (or unset)

update command test based on option change

ddcd57f

scheibelp closed this Sep 11, 2017

scheibelp reopened this Sep 11, 2017

merge from develop

6c3862d

tgamblin requested changes Sep 28, 2017

View reviewed changes

restore old --run-tests option, give it the same meaning as --test=al…

f5b639e

…l, and make it mutually exclusive with the --test option

scheibelp added 5 commits September 29, 2017 10:52

update install cmd tests to inspect state of PackageTesting object. A…

9bd2928

…dd check for --run-tests

move mock package/repo definitions to conftest to make them generally…

8bd95a3

… available

add deprecation warning for --run-tests option

02d22d7

flake edit

7b73815

there are multiple conftests, import the correct one

6318429

tgamblin merged commit 9e7faff into spack:develop Sep 30, 2017

alalazo mentioned this pull request Sep 30, 2017

module files: restricted token expansion + case sensitivity #5474

Merged

davydden mentioned this pull request Sep 30, 2017

allowing user to manuall run large test suite after spack install #2060

Closed

This was referenced Oct 3, 2017

gcc broken: mpfr build fails due to inappropriate libgmp, SPACK_LINK_DEPS missing? #5564

Closed

Fix environment dependency collection bug #5586

Merged

tgamblin added this to the v0.11.0 milestone Nov 12, 2017

adamjstewart mentioned this pull request Jan 1, 2018

Py-fparser: python parser for Fortran code #6794

Merged

adamjstewart mentioned this pull request Apr 8, 2018

Fix GCC 6/7 build on APFS #7696

Merged

adamjstewart mentioned this pull request Sep 16, 2019

Add test dependencies to list of RPATHed dependencies #12842

Closed

Conversation

scheibelp commented Aug 16, 2017

Uh oh!

davydden commented Aug 16, 2017

Uh oh!

scheibelp commented Aug 18, 2017

Uh oh!

adamjstewart commented Aug 18, 2017

Uh oh!

davydden commented Aug 18, 2017

Uh oh!

scheibelp commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naromero77 commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp commented Sep 11, 2017

Uh oh!

tgamblin commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Sep 28, 2017

Uh oh!

tgamblin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp commented Sep 28, 2017

Uh oh!

naromero77 commented Sep 29, 2017

Uh oh!

scheibelp commented Sep 29, 2017

Uh oh!

tgamblin commented Sep 29, 2017

Uh oh!

scheibelp commented Sep 29, 2017

Uh oh!

tgamblin commented Sep 30, 2017

Uh oh!

tgamblin commented Sep 30, 2017

Uh oh!

naromero77 commented Oct 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

scheibelp commented Aug 18, 2017 •

edited

Loading

tgamblin commented Sep 28, 2017 •

edited

Loading