Tests should make MDAnalysis raise exceptions

[jbarnoud]

Looking at #592, and where the exclamation marks in formatted string may be an issue, I realized we rarely test the cases where exceptions are raised. These parts account for a significant part of the uncovered code. While it could seem harmless, it means we are not sure the expected exception is raised for a given error. Also, we are not sure the code to raise the exception is correct as it is the case in #592.

Tests should account for bad inputs and case MDAnalysis to raise its exceptions.

[orbeckst]

Good idea. However, it's a pretty big task and although not very difficult, a bit daunting. It would be better to split it into more well defined tasks such as "tests for lib.transformations should check exception handling". This will (hopefully) make it more likely that someone picks up an issue (and it would also make it easier for new contributors to contribute).

New sub issues should reference this issue as a "parent" issue.

[richardjgowers]

Yep, definitely something that needs doing when writing tests.

The guide on writing tests probably needs to get spun out into its own page (separate from details on running them). And then an example of how both good & bad usage should be tested.

[orbeckst]

Anyone applying for GSoC and looking for a fixable issue should look at this one. Follow @fiona-naughton 's lead and identify one module or even just one class where we don't test an exception:

1. go to the coverage report
2. click on the build number and
3. under FILES, click on the All tab
4. pick a file name and click on it
5. search for red lines with except and identify file and class/function

Then find the test file under testsuite/MDAnalysisTests that tests the code that you identified.

1. Raise an issue in the tracker for the specific exceptions that you want to test (and mention this issue Tests should make MDAnalysis raise exceptions #597)
2. Add a test or tests that raise the exception.
3. Submit a PR (referencing your new issue).

[CalebMadsen]

Hey there,

I really appreciate you spelling things out for me. I went to the coverage report and just clicked the top build number (1700). I clicked through a few files and found this cone : https://coveralls.io/builds/5512450/source?filename=miniconda%2Fenvs%2Fpyenv%2Flib%2Fpython2.7%2Fsite-packages%2FMDAnalysis%2Fcore%2F__init__.py .
At about the 218th line there is an Except RaiseError. Is this what I'm looking for? If so the Class says Flag(object). I am not sure I understand how to use the tests. I tried this in Python:

importMDAnalysis.tests
MDAnalysis.tests.test()

And it returns a failure. I'm not sure what MDAnalysis.tests.test() does though. If you have any advice I 'd appreciate it. Thanks.

[jbarnoud]

This is indeed the kind of lines we would like the tests to cover. Ideally, we would like you to choose a file and have the tests cover all the exceptions it can raise. The file you chose is a good candidate, yet we plan to remove the flag feature (#782), so it may not be the most useful to tackle.

Explore the wiki. It has a full page on how to run and write tests.

[vedantrathore]

@kain88-de @jbarnoud I would like to work on this, could anyone give me a pointer on how to start with this?

[jbarnoud]

This issue is all about increasing the test coverage. Too often we write tests to make sure what should work does, but we omit the tests that make sure the case that should fail do fail as we expect. You can find the test coverage on our coverall page: <https://coveralls.io/github/MDAnalysis/mdanalysis> There is a project on github that tries to follow what is being done to reach 90% coverage (the project does not seem super up to date, though): <https://github.com/MDAnalysis/mdanalysis/projects/2> Anyway, the wiki guide for developers is a good read: <https://github.com/MDAnalysis/mdanalysis/wiki/Guide-for-Developers>. In your case, the page about writing tests is the one not to miss: <https://github.com/MDAnalysis/mdanalysis/wiki/UnitTests>. Hopefully it is up to date... Have fun!

…

On 15/01/17 00:26, Vedant Rathore wrote: @kain88-de <https://github.com/kain88-de> @jbarnoud <https://github.com/jbarnoud> I would like to work on this, could anyone give me a pointer on how to start with this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#597 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUWuj47bga8MOzW2DL4ZSRxlUTC9F3xks5rSVmjgaJpZM4G2Yjr>.

[kain88-de]

Failing as expected is done by either raising an exception or issuing a warning. To check that
an exception is thrown correctly you can use assert_raises examples can be found by looking for that function in the codebase

https://github.com/MDAnalysis/mdanalysis/search?utf8=%E2%9C%93&q=assert_raises&type=Code

Warnings can be checked with assert_warns

https://github.com/MDAnalysis/mdanalysis/search?utf8=%E2%9C%93&q=assert_warns

Finding places that aren't covered yet can be done using the coveralls report as @jbarnoud said.

To test for coverage locally you can run the tests with

./mda_nosetests --with-coverage --cover-erase --cover-html-dir=htmlcov --cover-html --cover-package=MDAnalysis <file/folder>

This will generate a html index file in htmlcov that you can open in your browser.

[vedantrathore]

@kain88-de @jbarnoud I Found this line, now I want to write a unit test for testing this line, How should I do that, I have already read the unit test doc, Just need some more clarifications

[jbarnoud]

The line you point too appears in green on coverall. This means that the line is covered by the tests. In particular, I think it is covered by test_psa.TestPSAExceptions.test_get_path_metric_func_bad_key.

[vedantrathore]

@jbarnoud Is this line Okay?

[jbarnoud]

@vedantrathore Yes. The whole function is untested; tests for this function should follow each branch (i out of bond, j out of bond, i and j in the expected range, i and j in the symmetric image of the matrix... For each branch, the function should raise the expected exception, issue the expected warning, or return the expected result.

There are also a few edge cases that could be tested. From the top of my head: bogus N < 0, bogus i < 0 or j < 0, i == 0 and j == 0, i == j.

[orbeckst]

On 9 Mar, 2017, at 06:22, Mohit Rathore ***@***.***> wrote: There has been a significant stir about this issue. Does this issue still need fixing?

Issue #597 is a "catch all" issue: it asks for tests that test exception handling in the code (see the discussion at the top of the issue). Yes: it still needs fixing. Open a new issue for a specific piece of code that needs a test, reference #597 in that issue report, and then specific discussions can be confined to your issue report. You would then submit a PR that references your issue.

[wvdtoorn]

In the spirit of letting other GSoC candidates know what is already being worked on, just wanted to inform that I'm currently working on improving the coverage of the exception raises in analysis/density.py.

[IAlibay]

@wvandertoorn I would have a look at PR #2537 before doing too many test changes for the density code. @orbeckst did quite a few changes to the tests there, so it might be that you find there is duplicaton of efforts for some things.

[orbeckst]

@wvandertoorn you can do tests for analysis.density and they are still welcome because even though PR #2537 will be a substantial rewrite, all the code you currently see will still be in 1.0.

However, there are a few final comments to address in PR #2537 and you could also make a PR onto PR #2537 and finish it (see #2537 (comment)). I quickly added comments on @IAlibay 's review comments. I would count finalizing and merging PR #2537 as fulfilling our requirements for GSoC applications. (Of course, you're then more than welcome to work on other things!!)

Probably the most challenging part is to figure out how to submit your own PR (from your fork) that references the PR #2537 .

[aya9aladdin]

Hello everyone
I'm planning to apply for google summer of code this year, and I'd like to contribute with a test case to be my first pull request to MDanalysis. I've read all the above instructions and the related links, but I'm a little confused by the coverall link mentioned above by @jbarnoud. I couldn’t find files having the uncovered parts underlined with the except word, as described above by Oliver. Could anyone help me with this?

[lilyminium]

Sorry for the confusion, @aya9aladdin. This is quite an old issue and afaik we've moved away from using coverall but now use codecov: https://codecov.io/gh/MDAnalysis/mdanalysis/tree/develop/package/MDAnalysis

If you click around on those and look for low percentage of coverage, that indicates that code is not being tested. Please ignore the due.py file, as that's not really part of our library.

[aya9aladdin]

ok I will check this, Thank you!