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

Track multiple processes on Travis only for build tests #6872

Merged

Conversation

alalazo
Copy link
Member

@alalazo alalazo commented Jan 9, 2018

On a local workstation, it seems that tracking multiple processes during coverage may result in malformed coverage reports for unit tests and not for build tests.

Given that multiple processes make a difference in coverage mainly for build tests, try to disable the tracking for unit tests to see if we get more stable coverage results.

On a local workstation, it seems that tracking multiple processes during
coverage may result in malformed coverage reports for unit tests and not
for build tests.

Given that multiple processes make a difference in coverage mainly for
build tests, try to disable the tracking for unit tests to see if we get
more stable coverage results.
@alalazo alalazo added tests General test capability(ies) travis labels Jan 9, 2018
@alalazo
Copy link
Member Author

alalazo commented Jan 9, 2018

@tgamblin @adamjstewart To extend a bit the explanation on the logic of this PR.

Using --concurrency=multiprocessing serves the purpose of tracking the coverage of code executed in spawned processes. I noticed today, while debugging the sudden drops in coverage, that running:

$ coverage run bin/spack test

on my laptop was sometimes resulting in a couple of malformed files (over the hundreds that were produced by the command). These malformed files are, I think, what sometimes prevent Travis from generating a well formed .coverage report to submit to codecov.

I also noted that build tests, like:

$ coverage run bin/spack install mpich

apparently didn't suffer from the same problem. So, while it's not really correct from a theoretical point of view, I thought we could disable tracking child processes in unit tests - i.e. where it matters the least - in favor of more stability in coverage numbers.

At a first glance this works, as we have all the 10 expected reports uploaded to codecov. The price is a fictitious decrease in coverage for a few modules in llnl that are used in child processes.

TLDR If you think this is worth it, the PR is for me ready to be merged.

@adamjstewart
Copy link
Member

Stability of coverage reports > lines of code covered

I'll merge this for now. Hopefully we can get to the bottom of why concurrency is a problem for the unit tests. If llnl is the only place where coverage drops, it should be easy to add unit tests for these functions.

@adamjstewart adamjstewart merged commit 596d463 into spack:develop Jan 10, 2018
@alalazo alalazo deleted the qa/multiprocessing_only_on_build branch January 10, 2018 16:16
alalazo pushed a commit to epfl-scitas/spack that referenced this pull request Jan 16, 2018
alalazo added a commit to epfl-scitas/spack that referenced this pull request Jan 16, 2018
alalazo added a commit to epfl-scitas/spack that referenced this pull request Jan 18, 2018
alalazo added a commit to epfl-scitas/spack that referenced this pull request Jan 18, 2018
alalazo added a commit that referenced this pull request Jan 20, 2018
* Revert "Travis: use --concurrency=multiprocessing only on build tests (#6872)"

This reverts commit 596d463.

* Removing 'coverage combine' in test script

According to what was discovered in #6887, one of the problems is
calling 'coverage combine' twice without the '-a' flag. This removes
the first call within our test scripts.
ch741 pushed a commit to ch741/spack that referenced this pull request Apr 20, 2018
…6872)

On a local workstation, it seems that tracking multiple processes during
coverage may result in malformed coverage reports for unit tests and not
for build tests.

Given that multiple processes make a difference in coverage mainly for
build tests, try to disable the tracking for unit tests to see if we get
more stable coverage results.
ch741 pushed a commit to ch741/spack that referenced this pull request Apr 20, 2018
* Revert "Travis: use --concurrency=multiprocessing only on build tests (spack#6872)"

This reverts commit 596d463.

* Removing 'coverage combine' in test script

According to what was discovered in spack#6887, one of the problems is
calling 'coverage combine' twice without the '-a' flag. This removes
the first call within our test scripts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests General test capability(ies) travis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants