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

test out coveralls submission #320

Merged
merged 18 commits into from
Mar 31, 2017
Merged

Conversation

nickhand
Copy link
Member

No description provided.

@nickhand
Copy link
Member Author

@rainwoodman -- I've got the coverage working, with a bit of a hack around the mpi4py_test framework. We can't use the --coverage option of runtests.py that comes with numpy's Tester class. Each process will write to the same .coverage file. We want to take advantage of the -p option for the coverage command, which appends the PID to each .coverage file name, but to do this, we need to use the coverage run runtests.py .... syntax which breaks the --mpirun syntax we had been. I came up with a workaround by just directly calling the mpirun command, which seems to work fine. We could change the behavior of mpi4py_test to handle coverage better, but this seems to be working for now.

@rainwoodman
Copy link
Member

Does it make sense to have one .coverage file per process? Looks like if we can change line 309 of mpi4py_test/__init__.py to give a different dst directory for each rank. Would that work with coverall?

@nickhand
Copy link
Member Author

I don't think that would work. The .coverage file for each process is written to the current working directory, in this case build/test. I think that dst directory is just for the html coverage reports, which we don't need in this case.

The nose class is limited in terms of supporting coverage (I think nose is no longer maintained?)....there doesn't appear to be any easy way to change the .coverage file name, for example.

@rainwoodman
Copy link
Member

Indeed. nose is no longer maintained. I am not sure numpy's plan about this. I think it is relatively easy to follow their choices though.

Does it work if you put parallel = True in coveragerc? The docs suggest it is the same as -p.

@nickhand
Copy link
Member Author

Unfortunately, it does not work if you do that...I think the coveragerc is ignored by the call to nosetests by numpy

@rainwoodman
Copy link
Member

OK. I think your current coverage.py driver is the cleanest way of using mpi4py_test with coverage.

Is it desirable to set up travis such that the coverage is run as one item of the test matrix, or only run when the commit message has a '[ci.coverage]' string (like [ci skip]).

I think there are benefits keeping the regular way of running the tests in travis. I also recall being reocmmended not to run coverage tests every time because it can get slow.

@nickhand
Copy link
Member Author

I think this is ready to merge now, @rainwoodman.

Remaining item is whether to compute coverage every time...seems to be okay for now. Perhaps we reassess if it becomes too slow

@rainwoodman
Copy link
Member

Sure. Let's merge this.

@rainwoodman rainwoodman merged commit 4f15cee into bccp:master Mar 31, 2017
@nickhand nickhand deleted the add_coverage branch July 5, 2017 19:38
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

Successfully merging this pull request may close these issues.

2 participants