[RFC] what test to add for an issue

[Chris Smith]

At https://github.com/sympy/sympy/pull/7945 discussion involving a change that I made to a test that was added in response to issue 6533 has arisen. In that issue a particular expression (which is slow to compute) raised an assertion error, identifying that a change needed to be made in the code. The code was fixed and that original slow expression and 2 others were added as tests. 

I noticed this issue while waiting for a local test to run. Specifically, I noticed that the slow test was taking about 2 minutes. I went back in history to where the change was made, backed up to the previous commit, and experimented with simpler expressions (similar to the one identified in the issue) that gave the same error. I replaced the original tests with that expression.

The question that has come up is whether the original error-producing expression needs to appear in the test suite. I think not and offer the following guide for adding tests for code modified after a problem has been identified:

1) use the original failing expression unless a much simpler expression will exhibit the same failure or a similar expression will run much faster; if the original expression runs quickly and is not too complicated, it is not necessary to spend much time trying to find something simpler.

2) add any additional tests needed to make sure that all the code added is covered.

The reason for not using the original expression is that in debugging a problem one often finds what the key failing issue is and can pinpoint that portion of code with a simpler test.

I just happened across this issue and went back to the commit before the commit that fixed the issue (0fa7b9730e56759e5f4bc3752143f5) and found a simpler expression that failed in the same way as the original expression and covered the few lines that were added in the fix. On my computer it cuts about 2 minutes off the suite test time. I would like to make this change in PR https://github.com/sympy/sympy/pull/7945 . It has been requested that somebody else confirm or reject my proposal.

[Aaron Meurer]

I think such situations are OK, as long as you understand why the test
failed before and verified that the new test really tests the same
thing.

Aaron Meurer

> --
> You received this message because you are subscribed to the Google Groups
> "sympy" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to sympy+un...@googlegroups.com.
> To post to this group, send email to sy...@googlegroups.com.
> Visit this group at http://groups.google.com/group/sympy.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/sympy/b9d1dc4d-6f16-4b69-ae74-edc14f18a2f3%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Sergey Kirpichev]

On Wednesday, September 3, 2014 6:44:06 AM UTC+4, Aaron Meurer wrote:

I think such situations are OK, as long as you understand why the test
failed before and verified that the new test really tests the same
thing.

1. We can make mistakes here.
2. Probably, we want to be sure, that we never break again the
fixed example (incl. by different reason).  How we can be sure about
this, if we don't preserve the original test?

> I noticed that the slow test was taking about 2 minutes.

Can't we exclude slow tests with some cli switch?

[Joachim Durchholz]

What good is a unit test that is excluded?

[Chris Smith]

On Wednesday, September 3, 2014 2:07:45 PM UTC-5, Sergey Kirpichev wrote:

On Wednesday, September 3, 2014 6:44:06 AM UTC+4, Aaron Meurer wrote:

I think such situations are OK, as long as you understand why the test
failed before and verified that the new test really tests the same
thing.

1. We can make mistakes here.

I think that might apply to replacing things in current master, but if you go back to the point in history where the fix was not made and have another example that fails in the same way, there is no reason to include both failing tests just as one would not try to find all possible failing cases and add them.

 

2. Probably, we want to be sure, that we never break again the
fixed example (incl. by different reason).  

This is what I would refer to as serendipitous coverage. If, today, we rewrote sympy from scratch, the existing tests would identify errors but would not necessarily be important to keep. As I see it, there are different types of tests that: 

- safeguard against conceptual errors (e.g. var('x', complex=True, real=False).is_imaginary is None)

- provide simple coverage, verifying that code is free of conceptual errors

- test functionality that should be present (like keyword handling)

 

The hard part about writing a good test suite is thinking of all the significantly different ways that a code path can be traversed. e.g. if a number is being handled, what aspects of "number" are important to test, e.g. positive, negative, imaginary, etc... As I have recently found, sometimes a complex number itself is not enough, you have to think about the arg of the number.

[Joachim Durchholz]

I think unit tests serve these purposes:

- Document an API by example. (We do that in doctests.)

- Guard against regression. (Normal tests.)

- Test emergent properties, i.e. those that cannot be trivially deduced
from reading the source code. (Doctest or normal test, depending on
whether the test is also documentation-worthy or not.)

They can't make 100% sure that the code is valid, they can only increase
confidence.
And since we're in a grey area, we can weigh: Is the added effort to run
and possibly maintain a unit test worth the increased confidence in the
code's validity?

[Sergey B Kirpichev]

On Thu, Sep 04, 2014 at 07:54:32AM +0200, Joachim Durchholz wrote:
> - Guard against regression. (Normal tests.)

If the SymPy fails
on the same, already reported code sample again (no matter for
which reason) - is that a regression?

[Joachim Durchholz]

Depends on whether you look at it intrinsically or extrinsically.

Extrinsically, it's a regression.

From an intrinsical perspective:
If it's the same reason, then it's a regression.
If it's a new reason, it's a new problem that happens to be triggered by
the same code sample.

I wouldn't place too much weight on definitions though.
The most important point here is: What decision gives the best
effort-to-effect ratio?

Personally, I do not like slow tests. Practice has shown that they tend
to be ignored, and they aren't even always run before a merge. So either
the effort is high, or the effect is low.

I'd actually like to see slow tests discouraged, as a policy decision.
I think slow tests are acceptable where the test is really important, or
part of some in-progress development effort where performance is
expected to go up.
For regressions, they're simply not worth it, at least in my eyes.

Actually, some people advocate eliminating unit tests if they have been
exercized so thoroughly that they're unlikely to ever trigger again:
their information value has decreased, and they hamper architectural
changes because they tend to need adaptation.
I haven't seen such a policy implemented and can't judge its good and
bad points. Also, SymPy is dealing in mathematical truths, so unit tests
probably won't ever become logically irrelevant (though they could
become technically irrelevant: a regression test might have been
triggered by failures in code that's long gone, so the regression test
does not fulfill any useful purpose anymore).

[Sergey B Kirpichev]

On Thu, Sep 04, 2014 at 08:47:58AM +0200, Joachim Durchholz wrote:
> Personally, I do not like slow tests. Practice has shown that they
> tend to be ignored, and they aren't even always run before a merge.

AFAIK, we check slow tests before merge (at least for reduced
number of versions of the python interpreters and so on).

[Aaron Meurer]

Marking the existing test as @slow and adding the faster test seems
like a good workaround to me.

Aaron Meurer

> --
> You received this message because you are subscribed to the Google Groups "sympy" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to sympy+un...@googlegroups.com.
> To post to this group, send email to sy...@googlegroups.com.
> Visit this group at http://groups.google.com/group/sympy.

> To view this discussion on the web visit https://groups.google.com/d/msgid/sympy/20140904121225.GA9964%40debian.

[Chris Smith]

At https://github.com/sympy/sympy/issues/7245, this discussion has arisen again. As long as the added test is fast it's not that big of an issue but it is one that could be addressed with a written policy regarding adding tests so we don't have to reiterate that issues which are noticed to no longer be broken should not be added to the test suite if the underlying issue is clear and the resolving PR tests that issue.

But as Joachim points out, there is an effort/effect ratio to consider. Perhaps it is easier to add a fast test than to find the resolving PR and understand what the problem was. Anyway, stating this (perhaps on the workflow) would be a good idea.

[Chris Smith]

new page added to wiki: https://github.com/sympy/sympy/wiki/Closing-an-Issue

[Aaron Meurer]

What about duplicate issues?

Aaron Meurer

> --
> You received this message because you are subscribed to the Google Groups
> "sympy" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to sympy+un...@googlegroups.com.
> To post to this group, send email to sy...@googlegroups.com.
> Visit this group at http://groups.google.com/group/sympy.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/sympy/63d48bce-0d23-41e4-a43e-9317a41aa074%40googlegroups.com.