travis: skip unit and build tests for package-only PRs by alalazo · Pull Request #5842 · spack/spack

alalazo · 2017-10-20T08:57:33Z

TLDR

With this PR all the branches whose name starts with packages will skip unit tests and build tests, on the promise that there are no modifications to core files. This promise is checked by an additional test that is run after flake8 and documentation.

Documentation on the new Travis features here.

Example

You can check that Travis in this PR run all the tests as it did before. In #5840 the modifications are basically the same but the branch is named packages/ldc_and_faster_ci. The resulting CI is here and it fails as expected.

closes #5180
closes #5822
closes #5840

For #5822: I started with the idea of fixing that issue and skipping unit + build tests. I ended up modifying part of core, hence the current status. If the 2 lines change in ldc + ldc-bootstrap bother people I can extract them.

alalazo · 2018-01-08T14:11:17Z

@scheibelp @tgamblin Do you have time to review this one? It may help reducing the cycles spent on testing when the PR modifies only packages. We need to "educate" users to start PRs of this kind on branches named packages/*, but I think it might be worth it...

tgamblin

@alalazo:

This looks awesome. The only thing I would request be changed is that we not completely eliminate unit tests for package builds, but drastically reduce them.

There are two unit test that actually matter for package builds:

In package_sanity.py there are two relevant tests:
1. test_get_all_packages: makes sure all packages are importable.
2. test_all_versions_are_lowercase: tests that all package names are lowercase (should probably be renamed -- not sure how this got crufty)
In python_version there is one relevant test:
1. test_package_module_compatibility: tests that package code is compatible with different python versions.

I think if we ran one test on Linux to do just these, we'd be in pretty good shape. What should probably happen is that we should add a pytest marker to them, then run spack test and filter on the marker (e.g. @ptyest.mark.packagetest).

Note that the python_version test is not 100% comprehensive, so we may still miss some python version compatibility issues that we would catch if we actually ran it for all versions of python. But I think it's pretty good. Note also that the package tests get all packages, so if you want to be really efficient you could only have it try to get (import) the packages that are added/modified in the PR. That is probably unnecessary, given how fast the tests run compared to the launch overhead.

Obviously, we also want the packages to actually be built on each PR, but that is something we're working on that needs to be done outside travis. The plan is to add that later.

Thoughts?

adamjstewart · 2018-01-18T18:41:31Z

👍 to what @tgamblin said, apparently you can't react to reviews like you can comments.

alalazo · 2018-01-19T11:48:27Z

@tgamblin @adamjstewart Done with the requested changes

Note that the python_version test is not 100% comprehensive, so we may still miss some python version compatibility issues that we would catch if we actually ran it for all versions of python. But I think it's pretty good.

I agree. In any case, if an issue slips through the reduced set of tests (quite rare occurrence imo), it will be caught immediately on merge to develop.

Obviously, we also want the packages to actually be built on each PR, but that is something we're working on that needs to be done outside travis. The plan is to add that later.

On that regard some self-advertisement for a PR: #6977 can be useful for the reporting phase of a build. Adding CDash (or whatever we want to use) on top of that should be as easy as adding a new template.

tgamblin

@alalazo ok reviewed again. Thanks for all this work!

I made some comments but the only concrete code change I have is to remove one test erroneously labeled packagetest. I have another question and maybe a change request, though.

I thought about it and it's a bit sad that travis isn't a bit more flexible about the conditions. It seems that they are pretty static -- we can't have Travis analyze the PR, determine what changed, and spawn different test cases based on that. This means we have to force users to name package branches certain things.

I am somewhat concerned that the rigid branch naming is going to confuse people and scare away contributors. If you look at our PR list right now, the base branch name of a lot of package PRs is not packages/*, but the scripts in this PR are pretty rigid about this and will fail builds.

What if, instead of failing PRs not properly labeled with packages/*, we let the regular tests run but comment on the PR that things could build much faster if you use the packages/ base name? The comment can be automatically added by the qa script, and it could link to the relevant contributor documentation. I think that would keep things rolling at first and would gradually get people to switch to the new naming scheme without scaring people off. Thoughts?

Another thing that might help would be to make it super easy to make a package PR, e.g. with a spack pull-request command or something like that. Then the checks and naming stuff could happen client-side and transparently to the user. Easybuild actually does some of this, though I am not sure they do it to control which tests run.

The most confusing way things can fail is if someone submits a package PR to one of the core build tests -- they probably don't know anything about those but we'll make them change the name of their PR branch still... is there a good way around this?

tgamblin · 2018-01-28T23:12:34Z

lib/spack/spack/test/python_version.py

    assert not all_issues


+@pytest.mark.packagetest


This one actually isn't needed is it? It doesn't look at package files, just core. Is there a reason to run this for package-only tests?

tgamblin · 2018-01-28T23:13:01Z

lib/spack/spack/test/python_version.py

    check_python_versions(pyfiles([spack.lib_path], exclude=exclude_paths))


+@pytest.mark.packagetest


I think just marking this one covers things.

tgamblin · 2018-01-28T23:18:24Z

share/spack/qa/no_changes_in_core.py

+
+# If something changed in the core libraries we need to test it
+core_path = os.path.join('lib', 'spack')
+changes_in_core = any(core_path in x for x in files)


For correctness I would make this any x.startswith(core_path) and make sure they're absolute paths. We don't have it now but I could imagine some mirror lib/spack structure arising somewhere outside the core (e.g. if we moved the tests or something) so I think it's best to be rigorous and do the real check.

tgamblin · 2018-01-28T23:21:01Z

share/spack/qa/no_changes_in_core.py

+
+if changes_in_relevant_packages:
+    print('FAILURE: at least one of the packages in the "Build tests" stage has been modified.')  # noqa: ignore=E501
+    print('Rename your branch to something that does not start with "packages"')  # noqa: ignore=E501


Rather than being obnoxious about naming, could we not just run the correct tests given the set of changes that we encounter? I kind of don't like that you have to name your branch something special to get the shorter/longer tests. Is this something that we have to do b/c of how stages work in travis?

Ok I read through this stuff. It looks like travis isn't particularly flexible about this so I guess we have to make sure the branches are named correctly like this. Can these conventions be made very clear in the contribution docs?

tgamblin · 2018-01-28T23:21:56Z

share/spack/qa/no_changes_in_core.py

+# Exit early because polling the repo may be slow
+if changes_in_core:
+    print('FAILURE: at least one core file has been modified.')
+    print('Rename your branch to something that does not start with "packages"')  # noqa: ignore=E501


See my comment below -- I think we shouldn't enforce package naming conventions so strictly. I think people are accustomed to naming their branches whatever they want in their own forks, as well.

Just a quick comment on this. The only failure case should be if somebody submitted a change to the core framework and called the branch packages/*. For the most common case where somebody changed just a package and named the branch whatever they want, that just triggers all the tests. It will be slower, but won't result in a failure because of naming.

👍 to the suggestion of sending an automated message to tell people they had a faster way to run tests, if they missed it.

tgamblin · 2018-01-28T23:23:15Z

share/spack/qa/no_changes_in_core.py

+#
+# Description:
+#     Helper script that returns 0 if any file in core
+#     has been changed, 1 otherwise


Could this script be a developer command instead of a standalone script? Seems like the stuff it reports is useful outside of this script (e.g., so users can know whether they pakage-only or core changes).

alalazo · 2018-01-29T21:01:15Z

I am somewhat concerned that the rigid branch naming is going to confuse people and scare away contributors. If you look at our PR list right now, the base branch name of a lot of package PRs is not packages/*, but the scripts in this PR are pretty rigid about this and will fail builds.

I left a comment above, but to reiterate:

if a package only PR is not named packages/* it will trigger the same tests as it does right now
if a PR with changes that are not only on packages is on a packages/* branch, that will fail

Said otherwise: you're allowed to get a fast test lane if you call your branch packages/*, but only if you don't modify core.

What if, instead of failing PRs not properly labeled with packages/*, we let the regular tests run but comment on the PR that things could build much faster if you use the packages/ base name?

That should be already the case (unless I miss something). The fact is that, with this configuration:

stages:
  - 'flake8 + documentation'
  - name: 'no changes in core'
    if: head_branch =~ ^packages
  - name: 'unit tests'
    if: NOT head_branch =~ ^packages
  - name: 'build tests'
    if: NOT head_branch =~ ^packages
  - name: 'unit tests - osx'
    if: type IN (cron)

we run the new python script only on branches that are called packages/*. Otherwise we'll run:

'flake8 + documentation'
'unit tests'
'build tests'

which is exactly the current situation. In any case, I like the idea of leaving a status message to inform people of changes to the qa workflow. Another way to help contributors could be a spack audit command, even though I wouldn't go as far as automating a pull-request. I see it more as a diagnostic command, like:

$ spack audit --base=develop

Modified files:
  var/spack/repos/builtin/packages/ldc-bootstrap/package.py
  var/spack/repos/builtin/packages/ldc/package.py

Naming: branch 'ldc_update' could be named 'packages/ldc_update' to skip unit tests on the PR

Flake8: checks were clean

...

If you agree I'd work on those ideas in later PRs.

The most confusing way things can fail is if someone submits a package PR to one of the core build tests -- they probably don't know anything about those but we'll make them change the name of their PR branch still... is there a good way around this?

You're right, but hopefully the error message:

FAILURE: at least one of the packages in the "Build tests" stage has been modified.
Rename your branch to something that does not start with "packages"

will be clear enough not to lose much time on the issue. I agree though that for the user it's an hassle.

alalazo · 2018-02-13T11:35:13Z

@tgamblin Is there any change needed here? Do you want me to move the script to a developer command within this PR?

alalazo · 2018-02-22T08:54:38Z

@tgamblin ping

alalazo · 2018-04-07T09:49:01Z

ping

alalazo · 2018-05-10T15:06:43Z

@scheibelp @tgamblin Travis seems to be flakey again, so maybe this can help with package only PRs?

For your convenience, here's an example from our fork.

alalazo · 2018-05-31T12:50:11Z

Is there still interest in this PR?

alalazo · 2018-08-14T13:51:11Z

@tgamblin

baberlevi · 2018-10-30T17:01:55Z

@tgamblin is this functionality in place? This PR is open, but I see this comment: #9403 (comment) that seems to indicate it might already work ?

I thought I'd seen some PRs from branches with this prefix skip the built tests, but I can't find any now.

With this PR all the branches that starts with 'packages' will skip unit tests and build tests, on the promise that there are no modifications to core files. This promise is checked by an additional test that is run prior to flake8 and documentation.

alalazo · 2019-10-21T06:36:30Z

Do we need to rebase this and bring it up to date? Mac OS builders on Travis are a real stopper when we receive a lot of package only PRs at the same time. This will reduce the request of MacOS builders in that case by ~50% (they'll be used on merge, but not on PR).

tgamblin · 2019-10-21T07:35:57Z

@alalazo:

Do we need to rebase this and bring it up to date? Mac OS builders on Travis are a real stopper when we receive a lot of package only PRs at the same time. This will reduce the request of MacOS builders in that case by ~50% (they'll be used on merge, but not on PR).

I'm kind of tempted to move to GitHub actions and have a seed stage that determines what to do based on what is actually modified. Travis's stages are pretty inflexible. What do you think?

alalazo · 2019-10-21T08:28:30Z

I'm kind of tempted to move to GitHub actions and have a seed stage that determines what to do based on what is actually modified. Travis's stages are pretty inflexible. What do you think?

If that's possible it would be better, since we won't impose a naming convention for branches to contributors. I need to have a closer look at Github actions but it seems they already support MacOS. Definitely worth a try. Which time-frame do you have in mind?

alalazo force-pushed the qa/faster_tests_for_prs_with_only_packages branch from 222cbbc to cec010d Compare October 20, 2017 09:07

alalazo added the don't-merge-yet label Oct 20, 2017

alalazo changed the title ~~travis: unit + build tests are conditional on branch name~~ travis: skip unit and build tests for package only PRs Oct 20, 2017

alalazo added ready tests General test capability(ies) and removed don't-merge-yet labels Oct 20, 2017

alalazo requested review from adamjstewart and tgamblin October 20, 2017 10:20

alalazo mentioned this pull request Oct 20, 2017

ldc fails to build #5822

Closed

alalazo force-pushed the qa/faster_tests_for_prs_with_only_packages branch from cec010d to c2f991c Compare October 24, 2017 09:09

alalazo mentioned this pull request Nov 22, 2017

Package only changes don't trigger build or unit tests #5180

Closed

alalazo requested a review from scheibelp November 23, 2017 20:48

alalazo mentioned this pull request Dec 5, 2017

ADIOS 1.13.0: LZ4 & blosc support #6572

Merged

alalazo force-pushed the qa/faster_tests_for_prs_with_only_packages branch from c2f991c to c7a2ae2 Compare January 11, 2018 09:46

alalazo mentioned this pull request Jan 11, 2018

Tracking changes of our perpetual release candidate epfl-scitas/spack#128

Closed

9 tasks

alalazo force-pushed the qa/faster_tests_for_prs_with_only_packages branch 2 times, most recently from f13da74 to 880e407 Compare January 11, 2018 10:18

This was referenced Jan 15, 2018

Can we reduce Travis build times #6928

Closed

Added flags to unit tests + OSX build done once per day #6988

Merged

tgamblin requested changes Jan 18, 2018

View reviewed changes

alalazo force-pushed the qa/faster_tests_for_prs_with_only_packages branch from 880e407 to a599376 Compare January 19, 2018 11:33

alalazo mentioned this pull request Jan 19, 2018

travis: unit + build tests are conditional on branch name #5840

Closed

This comment has been minimized.

Sign in to view

tgamblin changed the title ~~travis: skip unit and build tests for package only PRs~~ travis: skip unit and build tests for package-only PRs Jan 28, 2018

tgamblin requested changes Jan 29, 2018

View reviewed changes

alalazo force-pushed the qa/faster_tests_for_prs_with_only_packages branch from a599376 to 34b4e6b Compare January 29, 2018 20:34

alalazo mentioned this pull request Feb 28, 2018

Frequent errors on Travis build tests #7299

Closed

alalazo force-pushed the qa/faster_tests_for_prs_with_only_packages branch from 34b4e6b to 6ab7476 Compare March 19, 2018 08:47

alalazo force-pushed the qa/faster_tests_for_prs_with_only_packages branch from 6ab7476 to 74b2393 Compare May 31, 2018 13:02

alalazo force-pushed the qa/faster_tests_for_prs_with_only_packages branch from 74b2393 to a5d8506 Compare July 23, 2018 18:55

alalazo and others added 3 commits January 16, 2019 21:51

Run tests marked with 'packagetest' for package only PRs

d56f477

Use absolute paths to avoid issues if relocating files

0ff5d40

alalazo force-pushed the qa/faster_tests_for_prs_with_only_packages branch from a5d8506 to 0ff5d40 Compare January 16, 2019 20:52

Added SPDX License Header

3be87f0

alalazo mentioned this pull request Oct 29, 2019

Investigate Github actions as an alternative for CI #13485

Closed

alalazo mentioned this pull request Dec 23, 2019

Moved build tests from Travis to Github Actions #13967

Merged

tgamblin closed this in #13967 Dec 25, 2019

alalazo deleted the qa/faster_tests_for_prs_with_only_packages branch December 25, 2019 08:58

		check_python_versions(pyfiles([spack.lib_path], exclude=exclude_paths))


		@pytest.mark.packagetest

Conversation

alalazo commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TLDR

Example

Uh oh!

alalazo commented Jan 8, 2018

Uh oh!

tgamblin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamjstewart commented Jan 18, 2018

Uh oh!

alalazo commented Jan 19, 2018

Uh oh!

This comment has been minimized.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo commented Jan 29, 2018

Uh oh!

alalazo commented Feb 13, 2018

Uh oh!

alalazo commented Feb 22, 2018

Uh oh!

alalazo commented Apr 7, 2018

Uh oh!

alalazo commented May 10, 2018

Uh oh!

alalazo commented May 31, 2018

Uh oh!

alalazo commented Aug 14, 2018

Uh oh!

baberlevi commented Oct 30, 2018

Uh oh!

alalazo commented Oct 21, 2019

Uh oh!

tgamblin commented Oct 21, 2019

Uh oh!

alalazo commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alalazo commented Oct 20, 2017 •

edited

Loading

tgamblin left a comment •

edited

Loading

alalazo commented Oct 21, 2019 •

edited

Loading