Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#24261 closed enhancement (fixed)

py3: add py2 and py3 doctest flags

Reported by: embray Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords:
Cc: chapoton Merged in:
Authors: Erik Bray, Jeroen Demeyer Reviewers: Frédéric Chapoton, Erik Bray, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 12c300a (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

This adds two new flags that can be placed next to doctests: # py2 and # py3. If marked py2 a test will only be run on Python 2 and so on.

Generally this should be avoided in favor of making a test work on both in some way or another, but there are some cases where it simply doesn't make sense. For example this test in sage.doctest.control

 640             sage: DD = DocTestDefaults(sagenb = True)
 641             sage: DC = DocTestController(DD, [])
 642             sage: DC.add_files()  # py2
 643             Doctesting the Sage notebook.
 644             sage: DC.files[0][-6:]  # py2
 645             'sagenb'

This doesn't work on Python 3 since we don't even install the Sage notebook on Python 3 (at least not currently).

The relevant code being tested there should also be updated to handle Python 3 better, but even so there's no practical way to make the test make sense on Python 3 until/unless sagenb is supported on Python 3. I suspect other cases like this may come up on occasion so it's useful to have.

Change History (38)

comment:1 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:2 follow-ups: Changed 3 years ago by jdemeyer

Why not # optional - python2 or # optional - python3? That's reusing an existing system for optional tests.

comment:3 in reply to: ↑ 2 Changed 3 years ago by chapoton

Replying to jdemeyer:

Why not # optional - python2 or # optional - python3? That's reusing an existing system for optional tests.

and there are already existing instances waiting for that

comment:4 in reply to: ↑ 2 ; follow-up: Changed 3 years ago by embray

Replying to jdemeyer:

Why not # optional - python2 or # optional - python3? That's reusing an existing system for optional tests.

I considered something like that but it seems to have different semantics. Currently "optional - pkgname" implies that a package is installed/available. Here we don't care if python2/3 is available. We care about what Python version is currently running the tests.

Rather than making a special case it's simpler and clearer I think to just have specific tags for this (there's also prior precedence such as using # py3 in coverage to mark lines that should be ignored in the coverage analysis).

comment:5 Changed 3 years ago by tscrim

I think it would be better to have # py2 and # py3 behave like the # 32-bit and # 64-bit and be specific for the output. I guess there is also the question of how many of our doctest inputs are going to be specific for Python2. Do any of you have an impression? I think it might be marginal at worst, in which case, we can just build into the doctests an explicit check of Python2 vs 3 as part of the doctest itself.

comment:6 Changed 3 years ago by embray

I wouldn't mind changing it to apply to the output but not skip the test. That would actually be better since if the behavior on Python 3 changes in these cases (e.g. sagenb works, to use my previous example) then we'd want to see that reflected in the test results.

I think a stronger possibility is Python 3 only inputs, such as doing things with annotations and other Python 3 only syntax. So maybe having these tags work on both output and input would be good?

comment:7 Changed 3 years ago by tscrim

I would not oppose that, but I don't think we do anything like that elsewhere. So it might require some invasive changes and might cause some slight confusion (but that is debatable). I think we would be better off having a separate marker for Python3 input.

comment:8 Changed 3 years ago by embray

Adding tags on output lines would not require invasive changes, but I do think it's slightly more confusing. Whereas the use of comments on input lines is consistent with Python syntax, sticking comments in output lines is a bit ad-hoc and confusing. But we also already have precedence for that in the "32-bit/64-bit" case as you pointed out.

comment:9 Changed 3 years ago by embray

  • Status changed from needs_review to needs_work

I just realized there's a bug in how this is implemented, where tests containing these tags are skipped regardless.

comment:10 Changed 3 years ago by embray

Implementation bug aside, I've been working on fixing the sageinspect module, and that's increasingly convinced me that having a concise way to run specific test cases on either Python 2 or Python 3 only is useful, especially in that module.

comment:11 Changed 3 years ago by git

  • Commit changed from 6f5fd8cfb06c4b7c7f9743e2559eca00c06f3a0f to e01e5531b69e5a65642651efab85cf5bb496c8ee

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

e01e553Add doctest flags for running tests on Python 2 only or Python 3 only.

comment:12 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

Okay, this definitely works now. My fixes to the sageinspect module (and in particular its tests) rely on this pretty heavily.

comment:13 Changed 3 years ago by git

  • Commit changed from e01e5531b69e5a65642651efab85cf5bb496c8ee to 31f2008835ab51a4a1fdbe8b14b7ef175494be01

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

31f2008Add doctest flags for running tests on Python 2 only or Python 3 only.

comment:14 in reply to: ↑ 4 ; follow-up: Changed 3 years ago by jdemeyer

Replying to embray:

I considered something like that but it seems to have different semantics. Currently "optional - pkgname" implies that a package is installed/available. Here we don't care if python2/3 is available. We care about what Python version is currently running the tests.

Right, I get that. Still, I think that we already have enough (if not too many) special doctest markers. And even if # optional is typically used to check whether a package is installed, I think that the meaning of # optional - python2 is clear enough.

Moreover, there are already doctests using # optional - python2 and # optional - python3 referring to the current running version of Python.

comment:15 in reply to: ↑ 14 Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

I considered something like that but it seems to have different semantics. Currently "optional - pkgname" implies that a package is installed/available. Here we don't care if python2/3 is available. We care about what Python version is currently running the tests.

Right, I get that. Still, I think that we already have enough (if not too many) special

That's pretty subjective--I think "enough" is however many you need to write reasonable tests.

doctest markers. And even if # optional is typically used to check whether a package is installed, I think that the meaning of # optional - python2 is clear enough.

I don't. It's overly wordy and not quite clear at all given the double meaning.

Moreover, there are already doctests using # optional - python2 and # optional - python3 referring to the current running version of Python.

Except those explicitly don't work right now. I've already replaced a few examples using these markers. Here's a concrete example that might help motivate:

                sage: import ast, sage.misc.sageinspect as sms  # py3
                sage: visitor = sms.SageArgSpecVisitor()  # py3
                sage: vis = lambda x: visitor.visit_NameConstant(ast.parse(x).body[0].value)  # py3
                sage: [vis(n) for n in ['True', 'False', 'None']]  # py3
                [True, False, None]
                sage: [type(vis(n)) for n in ['True', 'False', 'None']]  # py3
                [<type 'bool'>, <type 'bool'>, <type 'NoneType'>]

This entire sequence of test commands is Python 3 only and does not even make sense on Python 2 (it might be nice to tag a whole range at once actually rather than every single line but that's a separate issue). Putting "# optional - python3" on every one of those lines is just that much more line noise.

comment:16 follow-up: Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

OK, fine. I don't really agree, but we have to move on. If you want to go forward with # py2 and # py3, you should at least fix the existing uses of # optional - python2 and # optional - python3. If you do that, I'm willing to set this ticket to positive review.

comment:17 in reply to: ↑ 16 Changed 3 years ago by embray

Replying to jdemeyer:

OK, fine. I don't really agree, but we have to move on. If you want to go forward with # py2 and # py3, you should at least fix the existing uses of # optional - python2 and # optional - python3. If you do that, I'm willing to set this ticket to positive review.

I'm fixing those as I come to them, but fixing all of them at once would probably only hang this up longer. Better to add the feature first and then switch to using it where it makes sense to on a case by case basis.

comment:18 Changed 3 years ago by jdemeyer

Come on, there are really few instances of # optional - python[23] currently in Sage. It doesn't take much effort to fix those. And those can also double as testcase that the # py2 and # py3 markers actually work.

comment:19 Changed 3 years ago by embray

Alright, I have no problem doing it (I'll have to double-check but I think I've already fixed most of those cases). It just seemed antithetical to your normal philosophy (which I generally agree with...)

comment:20 Changed 3 years ago by git

  • Commit changed from 31f2008835ab51a4a1fdbe8b14b7ef175494be01 to c09007614f985868bd2a43c0f8e282aeb8de17b7

Branch pushed to git repo; I updated commit sha1. New commits:

c090076Fix a few more cases where '# optional - python2' wasn't really doing what was intended.

comment:21 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

Note: I also fixed one case of # optional - python2 in #24286.

comment:22 Changed 3 years ago by chapoton

typo : "on optionl features" in src/doc/en/developer/coding_basics.rst

one failing doctest in src/sage/doctest/sources.py

comment:23 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

comment:24 Changed 3 years ago by embray

Can somebody explain the origin of this test?

sage -t --long src/sage/doctest/sources.py
**********************************************************************
File "src/sage/doctest/sources.py", line 759, in sage.doctest.sources.FileDocTestSource._test_enough_doctests
Failed example:
    for path, dirs, files in itertools.chain(os.walk('sage'), os.walk('doc')): # long time
        path = os.path.relpath(path)
        dirs.sort(); files.sort()
        for F in files:
            _, ext = os.path.splitext(F)
            if ext in ('.py', '.pyx', '.pxd', '.pxi', '.sage', '.spyx', '.rst'):
                filename = os.path.join(path, F)
                FDS = FileDocTestSource(filename, DocTestDefaults(long=True,optional=True))
                FDS._test_enough_doctests(verbose=False)
Expected:
    There are 7 tests in sage/combinat/diagram_algebras.py that are not being run
    There are 7 tests in sage/combinat/dyck_word.py that are not being run
    There are 7 tests in sage/combinat/finite_state_machine.py that are not being run
    There are 6 tests in sage/combinat/interval_posets.py that are not being run
    There are 18 tests in sage/combinat/partition.py that are not being run
    There are 15 tests in sage/combinat/permutation.py that are not being run
    There are 14 tests in sage/combinat/skew_partition.py that are not being run
    There are 18 tests in sage/combinat/tableau.py that are not being run
    There are 8 tests in sage/combinat/crystals/tensor_product.py that are not being run
    There are 11 tests in sage/combinat/rigged_configurations/rigged_configurations.py that are not being run
    There are 15 tests in sage/combinat/root_system/cartan_type.py that are not being run
    There are 8 tests in sage/combinat/root_system/type_A.py that are not being run
    There are 8 tests in sage/combinat/root_system/type_G.py that are not being run
    There are 3 unexpected tests being run in sage/doctest/parsing.py
    There are 1 unexpected tests being run in sage/doctest/reporting.py
    There are 15 tests in sage/manifolds/manifold.py that are not being run
    There are 3 tests in sage/rings/invariant_theory.py that are not being run
Got:
    There are 7 tests in sage/combinat/diagram_algebras.py that are not being run
    There are 7 tests in sage/combinat/dyck_word.py that are not being run
    There are 7 tests in sage/combinat/finite_state_machine.py that are not being run
    There are 6 tests in sage/combinat/interval_posets.py that are not being run
    There are 18 tests in sage/combinat/partition.py that are not being run
    There are 15 tests in sage/combinat/permutation.py that are not being run
    There are 14 tests in sage/combinat/skew_partition.py that are not being run
    There are 18 tests in sage/combinat/tableau.py that are not being run
    There are 8 tests in sage/combinat/crystals/tensor_product.py that are not being run
    There are 11 tests in sage/combinat/rigged_configurations/rigged_configurations.py that are not being run
    There are 15 tests in sage/combinat/root_system/cartan_type.py that are not being run
    There are 8 tests in sage/combinat/root_system/type_A.py that are not being run
    There are 8 tests in sage/combinat/root_system/type_G.py that are not being run
    There are 3 unexpected tests being run in sage/doctest/parsing.py
    There are 1 unexpected tests being run in sage/doctest/reporting.py
    There are 15 tests in sage/manifolds/manifold.py that are not being run
    There are 1 tests in sage/misc/sage_eval.py that are not being run
    There are 3 tests in sage/rings/invariant_theory.py that are not being run
    There are 1 tests in sage/rings/finite_rings/integer_mod.pyx that are not being run
**********************************************************************
1 item had failures:
   1 of   9 in sage.doctest.sources.FileDocTestSource._test_enough_doctests
    [367 tests, 1 failure, 244.00 s]

It's pretty awfully annoying to have a test whose output depends on the contents of a variety of specific modules. If this is meant to be some kind of regression test (?) it's not clear, but surely it could be run without such a strong dependency on external data.

comment:25 Changed 3 years ago by embray

I really don't understand what to do with this test. It's like it's testing the doctest parser by using a different, simpler doctest parser that doesn't fully work, and comparing the results from that parser to the results from the actual doctest parser. If we want to check that the actual doctest parser works then there should be (and are) unit tests for the code of the actual doctest parser. As it is this test is just testing the secondary, flakier doctest parser implemented in the test...

comment:26 Changed 3 years ago by git

  • Commit changed from c09007614f985868bd2a43c0f8e282aeb8de17b7 to 96127ebd7f23c774b4c5c6619962f8dcb8bc2ae5

Branch pushed to git repo; I updated commit sha1. New commits:

96127ebFix typo

comment:27 Changed 3 years ago by embray

  • Status changed from needs_work to needs_info

comment:28 Changed 3 years ago by jdemeyer

To solve your problem with _test_enough_doctests, you could add some extra attribute to SageDocTestParser to never skip any test (not even the # py2 and # py3 tests). Then you set that attribute when running _test_enough_doctests.

comment:29 Changed 3 years ago by embray

I'd still just as soon remove that test...

comment:30 Changed 3 years ago by jdemeyer

  • Branch changed from u/embray/python3/doctest-py2-only to u/jdemeyer/python3/doctest-py2-only

comment:31 Changed 3 years ago by jdemeyer

  • Authors changed from Erik Bray to Erik Bray, Jeroen Demeyer
  • Commit changed from 96127ebd7f23c774b4c5c6619962f8dcb8bc2ae5 to 12c300aad31ee72ea569134b35a94baed7efa470
  • Status changed from needs_info to needs_review

This variant re-uses the existing optional tags implementation. It passes all tests in sage/doctest.


New commits:

39c71a2Add doctest flags for running tests on Python 2 only or Python 3 only.
1fcea7eBetter fix for this doctest
12c300aAutomatically add py2/py3 optional tag

comment:32 follow-up: Changed 3 years ago by embray

I don't fully understand what this is doing differently, but I'm not opposed as long as it works.

comment:33 in reply to: ↑ 32 Changed 3 years ago by jdemeyer

Replying to embray:

I'm not opposed

Is that a subtle way to say "positive review"?

comment:34 Changed 3 years ago by chapoton

This seems to be ready to go, no ?

comment:35 Changed 3 years ago by chapoton

  • Reviewers set to Frédéric Chapoton, Erik Bray, Jeroen Demeyer
  • Status changed from needs_review to positive_review

ok, let it be. Erik and Jeroen, I added your names as reviewers. Feel free to undo that.

comment:36 Changed 3 years ago by embray

Yes, I understand how this is working now. +1

comment:37 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/python3/doctest-py2-only to 12c300aad31ee72ea569134b35a94baed7efa470
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:38 Changed 3 years ago by chapoton

  • Commit 12c300aad31ee72ea569134b35a94baed7efa470 deleted

follow-up in #24570

Note: See TracTickets for help on using tickets.