Use dont_inherit arg to compile in test runner

[oscarbenjamin]

References to other Issues or PRs

Fixes the test runner to avoid the situations seen in #15522 , #15579 , #15515

Brief description of what is fixed or changed

The test runner is changed to use importlib.import_module rather than compile/exec. This means that each test file is compiled as it would be if directly imported in an import statement.

The previous compile/exec code for this treated all files as utf8 even if no encoding declaration was present. This meant that the tests would pass under PY2 even if the test module was not normally importable.

EDIT: I've changed this patch and force-pushed. The new changes are: Pass flag=0, no_inherit=True arguments to compile in the test runner.

This means that test modules won't inherit the __future__ flags of the sympy.utilities.runtests module. The problem with files having no encoding declaration is unsolved by the latest version of this patch but that's not such a big problem anyway.

Previously compile implicitly injects the __future__ flags from the calling module into the code that is compiled. This means that all tests are implicitly run with from __future__ import division when under the test runner. This also can mean that the result of the test is different when run under the test runner compare to when imported and called directly.

I found a number of other integer division bugs in the tests as a result of this. The first commit is the changes to the test runner. The others are fixes to integer division in various test files.

Other comments

Release Notes

• utilities
  • Test runner no longer forces __future__ division so that the test behaviour is the same under the runner as when imported normally.

[sympy-bot]

✅

Hi, I am the SymPy bot (v142). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• utilities
  • Test runner no longer forces __future__ division so that the test behaviour is the same under the runner as when imported normally. (#15587 by @oscarbenjamin)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.4.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests .-->

Fixes the test runner to avoid the situations seen in #15522 , #15579 , #15515 

#### Brief description of what is fixed or changed

~~The test runner is changed to use `importlib.import_module` rather than `compile/exec`. This means that each test file is compiled as it would be if directly imported in an `import` statement.~~

~~The previous compile/exec code for this treated all files as utf8 even if no encoding declaration was present. This meant that the tests would pass under PY2 even if the test module was not normally importable.~~

EDIT: I've changed this patch and force-pushed. The new changes are: Pass `flag=0, no_inherit=True` arguments to compile in the test runner.

This means that test modules won't inherit the `__future__` flags of the `sympy.utilities.runtests` module. The problem with files having no encoding declaration is unsolved by the latest version of this patch but that's not such a big problem anyway.

Previously `compile` implicitly injects the `__future__` flags from the calling module into the code that is compiled. This means that all tests are implicitly run with `from __future__ import division` when under the test runner. This also can mean that the result of the test is different when run under the test runner compare to when imported and called directly.

I found a number of other integer division bugs in the tests as a result of this. The first commit is the changes to the test runner. The others are fixes to integer division in various test files.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* utilities
    * Test runner no longer forces `__future__` division so that the test behaviour is the same under the runner as when imported normally.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[smichr]

Thanks for your attention to these details. This looks good to me. Let's see if there are other comments.

[oscarbenjamin]

Looking more closely at this patch I think maybe it disables enhanced asserts. Are they used?

[oscarbenjamin]

Ironically the motivation from my perspective for this and the other patches is to make it possible to use pytest with sympy. Enhanced asserts are one of the major features of pytest. Having just tested them pytest's assert rewriting is significantly better than the bin/test version.

[asmeurer]

I use them sometimes. I suppose I could just switch to pytest when I need them, though I often find pytest's enhanced asserts annoying.

[asmeurer]

Due to pytest-dev/pytest#2256

[oscarbenjamin]

I think it's possible to rewrite this to fix the integer division issue only. I could change it back to using compile/exec but pass dont_inherit to compile as specified in https://docs.python.org/2/library/functions.html#compile.

The wouldn't detect problems with encoding but those are less of an issue anyway.

[oscarbenjamin]

Also pytest's assert rewriting is better in other ways.

If I add this to test_ode.py:

def test_stuff():
    eqs_sols = {
        'eq1': (Eq(f(x).diff(x), f(x)), Eq(f(x), C0*exp(x))),
        'eq1': (Eq(f(x).diff(x), 2*f(x)), Eq(f(x), C0*exp(x)))
        }
    for eqname, (eq, sol) in eqs_sols.items():
        assert dsolve(eq) == sol

Now run with bin/test:

$ bin/test -E sympy/solvers/tests/test_ode.py -k test_stuff
...
Traceback (most recent call last):
  File "/Users/enojb/current/sympy/sympy/sympy/solvers/tests/test_ode.py", line 33, in test_stuff
    assert dsolve(eq) == sol
AssertionError: 
Eq(f(x), C1*exp(2*x)) ==
Eq(f(x), C0*exp(x))
...

Under pytest I get the extra crucial information:

$ pytest sympy/solvers/tests/test_ode.py -k test_stuff
...
    def test_stuff():
        eqs_sols = {
            'eq1': (Eq(f(x).diff(x), f(x)), Eq(f(x), C0*exp(x))),
            'eq1': (Eq(f(x).diff(x), 2*f(x)), Eq(f(x), C0*exp(x)))
            }
        for eqname, (eq, sol) in eqs_sols.items():
>           assert dsolve(eq) == sol
E           assert Eq(f(x), C1*exp(2*x)) == Eq(f(x), C0*exp(x))
E            +  where Eq(f(x), C1*exp(2*x)) = dsolve(Eq(Derivative(f(x), x), 2*f(x)))

sympy/solvers/tests/test_ode.py:33: AssertionError
...

If I could see that kind of output on Travis I might now the problem with a patch without needing to have run the code on my machine.

[oscarbenjamin]

I've written a new patch that uses the dont_inherit argument to compile rather than using import_module.

This means that enhanced asserts still work and the test runner will pick up problems with integer division when running on Python 2.7. This version of the patch won't pick up problems with encoding declarations but that's not such a problem anyway.

[sidhantnagpal]

The division issue seems to be due to future_flags being passed to compileflags.

[oscarbenjamin]

The division issue seems to be due to future_flags being passed to compileflags.

Those are only used for the doctests.

For the normal tests compile is called here
https://github.com/sympy/sympy/blob/master/sympy/utilities/runtests.py#L1173 and no flags argument is provided. That means that the flags are inherited from the enclosing module which has from __future__ import division at the top. That's what is changed by this PR.

[sidhantnagpal]

+1 for the changes relating to unit tests.

[oscarbenjamin]

New division bugs are added to sympy continuously.

Here's two from two different PRs both merged 2 days ago: https://github.com/sympy/sympy/pull/15646/files#diff-da2e1fbc257d693653530689558aa08eR94
https://github.com/sympy/sympy/pull/15641/files#diff-1a4c5e7ea088dee2f708f144b19e99d8R475

These show up when running the tests under pytest with Python 2.7. I would like to make it so that pytest is usable with the SymPy tests but something needs to be changed to stop new division bugs from arriving.

I see two possibilities for this:

1. Add from __future__ import division to all tests. This allows new tests to arrive with integer division and still pass under Python 2 and 3 with either bin/test or pytest.
2. Ensure that the tests are not implicitly run with from __future__ import division (i.e. this patch). This way CI will pick up on these issues before they are merged.

My preference is this patch because it seems clear to me that these test fails are showing unintended mistakes.

Looking at the two examples above I'm fairly sure that what is intended is e.g. S(1)/5 rather than 1/5. If @normalhuman confirms then I'm happy to fix both in this PR. In some other cases I see that float is actually wanted in which case e.g. 1.0/5 would be better.

[asmeurer]

It's worth noting that we will drop Python 2 support in about a year https://github.com/sympy/sympy/wiki/Python-version-support-policy. But it would still be nice to have the tests somehow avoid usage of explicit integer division.

We could even consider a test that tests that nowhere does explicit integer/integer (require it to always be wrapped as a rational or require one of them to be a float).

[oscarbenjamin]

Python 2.7 has a specific command-line option to warn about integer division:

$ python -Qwarn
Python 2.7.10 (default, Oct  6 2017, 22:29:07) 
[GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.31)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> 2/3
__main__:1: DeprecationWarning: classic int division
0

It's been removed in Python 3 though.

I don't see any way to pick up on integer division in general without hooking into int.__truediv__ because it can be done without literals (there were many cases of this in #15515):

x = 1
y = 2
assert x / y == z

[asmeurer]

I guess a big part of the problem is that things like 1/5 == Rational(1/5) currently return True. That is itself a bit of a nightmare to fix properly (see #11707 and related issues). But once we do fix it this would be a less of a concern, because only floats that are exactly representable as their rationals (denominator is a power of 2) would even pass the tests in the first place. So if someone wrote 1/2 instead of S(1)/2 tests would still pass, but 0.5 as a float is exactly equal to 1/2, whereas 1/5 instead of S(1)/5 would make tests fail, because it would only compare equal to the rational 3602879701896397/18014398509481984. It would still be bad to use floats in a test where rationals are actually returned, but less bad than it is now.

[normalhuman]

I meant S(1)/5 and so on; sorry about being sloppy and Python3-centric.

[oscarbenjamin]

I've rebased and pushed another couple of commits to address the new division bugs. I'm happy with this PR now.

No need to be sorry @normalhuman: if you look at #15515 you can see that these were ubiquitous in the codebase and coming from all contributors. This mistake is too easy to be made because it is also made by repr:

>>> x ** (S(1)/3)
x**(1/3)
>>> repr(x ** (S(1)/3))
'x**(1/3)'

So any time someone copies the repr in to the tests this can happen.

[codecov]

Codecov Report

Merging #15587 into master will increase coverage by 11.57%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master   #15587       +/-   ##
===========================================
+ Coverage    61.6%   73.18%   +11.57%     
===========================================
  Files         618      618               
  Lines      157812   157812               
  Branches    37127    37127               
===========================================
+ Hits        97220   115494    +18274     
+ Misses      54706    36789    -17917     
+ Partials     5886     5529      -357

Impacted Files	Coverage Δ
sympy/utilities/runtests.py	35.04% <100%> (+1.06%)	⬆️
sympy/tensor/indexed.py	94.11% <0%> (-0.85%)	⬇️
sympy/combinatorics/coset_table.py	86.65% <0%> (-0.42%)	⬇️
sympy/combinatorics/perm_groups.py	76.97% <0%> (-0.4%)	⬇️
sympy/solvers/ode.py	64.86% <0%> (ø)	⬆️
sympy/logic/boolalg.py	95.16% <0%> (+0.1%)	⬆️
sympy/solvers/solveset.py	92.16% <0%> (+0.18%)	⬆️
sympy/functions/combinatorial/factorials.py	78.36% <0%> (+0.21%)	⬆️
sympy/core/evalf.py	88.03% <0%> (+0.22%)	⬆️
sympy/matrices/common.py	91.16% <0%> (+0.27%)	⬆️
... and 197 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f9d654...1bd2b3a. Read the comment docs.