Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix doctest for evaluate #7399

Closed
wants to merge 3 commits into from
Closed

Conversation

asmeurer
Copy link
Member

What's concerning here is that the doctest runner (obviously) isn't run on
this doctest.

What's concerning here is that the doctest runner (obviously) isn't run on
this doctest.
@@ -20,10 +20,10 @@ def evaluate(x):

>>> from sympy.abc import x
>>> from sympy.core.operations import evaluate
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this correct?
from sympy.core.operations doesn't work for me, from sympy.core.evaluate - does.

@skirpichev
Copy link
Contributor

What's concerning here is that the doctest runner (obviously) isn't run on
this doctest.

Indeed. Why we can't use py.test? It works:

$ py.test --doctest-modules sympy/core/evaluate.py
============================================================================= test session starts ==============================================================================
platform linux2 -- Python 2.7.3 -- py-1.4.20 -- pytest-2.5.2
architecture: 64-bit
cache:        yes
ground types: python 

collected 1 items 

sympy/core/evaluate.py .

=========================================================================== 1 passed in 0.05 seconds ===========================================================================

@smichr
Copy link
Member

smichr commented Apr 13, 2014

It's the decoration? If you put another function in the file its doctest will be tested; but not evaluate's.

@asmeurer
Copy link
Member Author

Does py.test run the doctest in its own namespace? I know python -m doctest does not.

@asmeurer
Copy link
Member Author

I just tested it and it doesn't. py.test acts the same as python -m doctest. The test is

def test():
    """
    >>> test()
    1
    >>> 2
    3
    """
    return 1

(the second test was to make sure the test was working at all). Both python -m doctest and py.test --doctest-modules don't give any error for the first line, whereas our doctest runner (you have to move it to sympy/test.py so it will find it):

File "./sympy/test.py", line 3, in sympy.test.test
Failed example:
    test()
Exception raised:
    Traceback (most recent call last):
      File "/Users/aaronmeurer/anaconda3/lib/python3.3/doctest.py", line 1313, in __run
        compileflags, 1), test.globs)
      File "<doctest sympy.test.test[0]>", line 1, in <module>
        test()
    NameError: name 'test' is not defined
**********************************************************************
File "./sympy/test.py", line 5, in sympy.test.test
Failed example:
    2
Expected:
    3
Got:
    2

This is a pretty important feature of our doctest runner. It means that all doctests have to be completely self-contained. Executing the doctest in the module namespace is only useful if you are writing a small script or something. In most cases, and especially in our case, functions won't be run in the same module namespace as they were defined.

@asmeurer
Copy link
Member Author

Regarding this, it's got to be the decoration. But we have a ton of decorated functions. Does this mean that none of them are being doctested?

@skirpichev
Copy link
Contributor

On Sun, Apr 13, 2014 at 10:59:06AM -0700, Aaron Meurer wrote:

Regarding this, it's got to be the decoration. But we have a ton of
decorated functions. Does this mean that none of them are being doctested?

It seems to be the case. For example:

$ cat >> sympy/core/evaluate.py <<EOF
def dummy_dec(f):
return None

@dummy_dec
def test111(x):
"""
>>> from sympy.core.evaluate import test111
>>> test111(1)
2
"""
return x + 1
EOF

$ ./bin/doctest sympy/core/evaluate.py
===================================================== test process starts ======================================================executable: /usr/bin/python (2.7.3-final-0) [CPython]
architecture: 64-bit
cache: yes
ground types: gmpy 1.15
hash randomization: on (PYTHONHASHSEED=3624908392)

========================================== tests finished: 0 passed, in 0.00 seconds ===========================================

Nothing was tested at all (should be a failure).

@asmeurer
Copy link
Member Author

I'm not sure if that's a fair test. The decorator should at least return a function. And anyway, @contextmanager is from the standard library, so it should be using @wraps, which copies over the docstring.

@skirpichev
Copy link
Contributor

On Sun, Apr 13, 2014 at 11:28:02AM -0700, Aaron Meurer wrote:

I'm not sure if that's a fair test.

Why? We should have an error:
In [2]: def empty(f):
...: None
...:

In [3]: @empty
...: def ff(x):
...: return x + 1
...:

In [4]: ff(2)

TypeError Traceback (most recent call last)
in ()
----> 1 ff(2)

TypeError: 'NoneType' object is not callable

Though, @decorated doctests seems to be working in the series
module. E.g. in residue.py. But not for @contextmanager.

@asmeurer
Copy link
Member Author

So maybe @wraps is the problem. timethis does not use it.

@asmeurer
Copy link
Member Author

@asmeurer
Copy link
Member Author

git blame gives 75e01c6. @thisch

@asmeurer
Copy link
Member Author

We should also look into removing that except Exception

@asmeurer
Copy link
Member Author

If I revert that commit the evaluate doctests are run.

@asmeurer
Copy link
Member Author

The issue that fixed is now #5046. I think if we can't figure this out then we should just revert that commit. It's better to have wrong line numbers than to not run doctests on decorated functions at all.

This reverts commit 75e01c6.

Conflicts:
	sympy/utilities/runtests.py
@asmeurer
Copy link
Member Author

I've pushed a commit to this branch that reverts that. If someone wants to look into fixing this better, please do so and disregard this pull request. Otherwise, this fix should be good enough.

@twmr
Copy link
Contributor

twmr commented Apr 13, 2014

@asmeurer I'll look into this in the comming days

@twmr
Copy link
Contributor

twmr commented Apr 13, 2014

This seems to do the trick:

--- a/sympy/utilities/runtests.py
+++ b/sympy/utilities/runtests.py

@@ -1500,7 +1500,11 @@ def _get_test(self, obj, name, module, globs, source_lines):
         if lineno is None:
             # handling of properties is not implemented in _find_lineno so do
             # it here
-            tobj = obj if not isinstance(obj, property) else obj.fget
+            if hasattr(obj, 'func_closure'):
+                tobj = obj.func_closure[0].cell_contents
+            else:
+                tobj = obj
             lineno = self._find_lineno(tobj, source_lines)

         assert lineno is not None

@asmeurer
Copy link
Member Author

Can you submit a pull request with that, if you have the time. We should also look into fixing that except Exception as I noted, and I wont be surprised if more doctests are failing with this than the evaluate one.

@asmeurer
Copy link
Member Author

Although Travis passes here, so maybe there aren't any other bad doctests.

@smichr
Copy link
Member

smichr commented Apr 14, 2014

We should definitely fix the line numbering issue, but it is more important that the tests work. Is this ready to go?

twmr added a commit to twmr/sympy that referenced this pull request Apr 14, 2014
supersedes sympy#7399

with this commit all decorated functions/methods containing doctests,
where the used decorator is != ``property`` are executed now and not
silently skipped.
@smichr smichr closed this in #7402 Apr 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants