search

MDAnalysis / mdanalysis

MDAnalysis is a Python library to analyze molecular dynamics simulations.
https://mdanalysis.org
Other
1.32k stars 652 forks source link

Drop in coverage of coordinates module #1433

Closed utkbansal closed 7 years ago

utkbansal commented 7 years ago

We have a drop in coverage for the coordinates module because my PR #1414 (for porting it to pytest) was merged by mistake.

It is to be noted that the error occurred because of coveralls showing a green signal even when it should have - there was an obvious drop in coverage. Maybe we need to look at how it is configured.

Solution

I will be opening a new PR soon to bring the coverage back to normal.

jbarnoud commented 7 years ago

There is a drop in coverage indeed, but it is very localized. I think we can live with it if the rest of the PR comes soon enough.

For the record, here are the number of tests run by nose and by pytest per file. I ran nose on the commit c27ace3e653d5189aaa1f9b1f005c04515aa02e6 that is just before your changes, and I ran pytest on commit 01bd06604a4236a60aecfa63bf71df1cf23c050f that contains your changes. The difference is pytest - nose; a positive would mean that pytest discover more tests than nose, but this is most likely due to my counting method that misses test that have a docstring when processing nose results.

Nose before Pytest after Difference File
254 124 -130 test_gro
81 3 -78 test_xyz
139 71 -68 test_xdr
48 0 -48 test_memory
61 30 -31 test_reader_api
158 156 -2 test_dcd
10 10 +0 test_mmtf
1 1 +0 test_null
11 11 +0 test_crd
11 12 +1 test_pdbqt
120 121 +1 test_dms
121 133 +12 test_pdb
129 130 +1 test_trz
14 15 +1 test_chainreader
144 148 +4 test_trj
147 149 +2 test_timestep_api
153 159 +6 test_dlpoly
16 16 +0 test_mol2
18 18 +0 test_gms
2 2 +0 test_netcdf
24 25 +1 test_pqr
3 3 +0 test_amber_inpcrd
4 4 +0 test_writer_registration
48 48 +0 test_lammps

I get the distributions with the following commands:

# For nose
./mda_nosetests --no-open-files --processes 2 --process-timeout 800 -v coordinates 2>&1 | tee log_nose
cat log_nose | grep -v SKIP | egrep -o 'testsuite\.MDAnalysisTests\.[^ )]+' | cut -f 4 -d . | sort | uniq -c > distrib_nose

# For pytest
pytest -v coordinates/ 2>&1 | tee log_pytest
cat log_pytest | grep -v SKIP | egrep '^coordinates/' | cut -f 1 -d . | cut -f 2 -d / | sort | uniq -c > distrib_pytest
kain88-de commented 7 years ago

There is no need to look much into DCD. We are doing a complete rewrite of it right now anyway.

kain88-de commented 7 years ago

@utkbansal I'd appreciate if you could prioritize this.

utkbansal commented 7 years ago

@kain88-de I'm already working on it. There should be a PR by tomorrow!

utkbansal commented 7 years ago

@kain88-de @richardjgowers @jbarnoud @mnmelo I'm stuck and need help with the test involving reference pattern. The normal convention I've been using so far isn't working for this. Plus one of the most frustrating things about working with pytest is that it is not showing me the original stack trace, the data that gets logged is almost useless. But I'm guessing this can be configured.

Currently, I'm trying to get it to give me the real stack traces and then I'll try to use fixtures to do this work.

kain88-de commented 7 years ago

put up a PR then it's easier to talk about issues.

utkbansal commented 7 years ago

Okay, I'll open up a PR for auxiliary, it has the exact same convention followed but is smaller and more manageable in comparison to coordinates module and should be easier to review and get feedback. Plus the coordinates module is kind of messy right now.

utkbansal commented 7 years ago

I can't find the reason for the drop in the number of tests collected in test_reader_api. My guess is that it has something to do with yield based tests. pytest counts them as a single test, maybe nose counts them as multiple?!

kain88-de commented 7 years ago

what does the coverage count say?

utkbansal commented 7 years ago

@kain88-de Will check once I'm done with test_xdr

Are we sure that I dont need to work on the dcd tests?

kain88-de commented 7 years ago

@utkbansal yes. I'm doing that in #1372. If you change anything now rebasing will be hell for me.

kain88-de commented 7 years ago

Having the tests for dcd also doesn't matter much since we are rewriting this completely

utkbansal commented 7 years ago

okay, don't want to create a mess for you.

utkbansal commented 7 years ago

@kain88-de Only ran test_reader_api for coverage. There is a 2% drop!!

EDIT: This might be wrong. Investigating.

jbarnoud commented 7 years ago

Be careful that coverage is not enough of a metric. Multiple tests can cover the same code but test different things.By grepping the verbose output of the tests you should be able to identify the test functions that you may be missing. Le 28 juin 2017 2:40 PM, Utkarsh Bansal notifications@github.com a écrit :@kain88-de Only ran test_reader_api for coverage. There is a 2% drop!!

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.