search

djay0529 / mdanalysis

Automatically exported from code.google.com/p/mdanalysis
0 stars 0 forks source link

Code review request #229

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Purpose of code changes on this branch:
Commit 2cc54050d77b, on develop.

When reviewing my code changes, please focus on:
The new 'mightpass' flag to the knownfailure decorator.
(testsuite/MDAnalysisTests/__init__.py)

Original issue reported on code.google.com by manuel.n...@gmail.com on 23 Mar 2015 at 8:16

GoogleCodeExporter commented 9 years ago 
The code looks like it works as intended, but it just means that pass or fail 
the test does nothing?

What might be preferable is a proper known failure error, so running tests 
would look like

....FF...KKK.....

So people know that known errors (and might passes) have been encountered.  
Otherwise they're invisible and liable to be forgotten.

Original comment by richardjgowers on 24 Mar 2015 at 10:13

GoogleCodeExporter commented 9 years ago 
No, it isn't always silent. 
If the knownfailure exception is raised, then it is marked as "K". If no 
exceptions are raised then it is marked as a "."

This is in contrast with the original behavior of knownfailure, which was to 
mark the exceptions as "K" but raise an exception if the test successfully 
passed.

Now, we're applying the same test over all timestep classes. It'll fail for 
most, but it'll pass for TRR. If left at knownfailure's original behavior we'll 
get a bunch of "K" and one "E" for, ironically, the only test that passed.
With "mightpass" we simply tell it not to raise errors if the test does pass.

Of course, this flag should never be set for a knownfailure that should always 
fail. It is useful debugging to be alerted when some code changes inadvertently 
fix knownfailures.

Original comment by manuel.n...@gmail.com on 24 Mar 2015 at 10:51

GoogleCodeExporter commented 9 years ago 
Ok, I think I understand it better now.

The "K" doesn't exist yet though (at least for me?). So I guess my real 
complaint is the knownfailure decorator should give a "K", which isn't what 
this code review is about... so everything is good :)

Original comment by richardjgowers on 24 Mar 2015 at 11:20

GoogleCodeExporter commented 9 years ago 
Ah, sorry, you're right: we now get a bunch of "S" and one ".".

Original comment by manuel.n...@gmail.com on 24 Mar 2015 at 11:24

GoogleCodeExporter commented 9 years ago

Original comment by orbeckst on 28 Mar 2015 at 12:22