py3: add py2 and py3 doctest flags

[embray]

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.

[jdemeyer]

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

[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

[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).

[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.

[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?

[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.

[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.

[embray]

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

[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.

[git]

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

​e01e553

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

[embray]

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

[git]

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

​31f2008

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

[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.

[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.

[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.

[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.

[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.

[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...)

[git]

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

​c090076

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

[embray]

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

[chapoton]

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

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

[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.

[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...

[git]

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

​96127eb

Fix typo

[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.

[embray]

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

[jdemeyer]

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

---

New commits:

​39c71a2	Add doctest flags for running tests on Python 2 only or Python 3 only.
​1fcea7e	Better fix for this doctest
​12c300a	Automatically add py2/py3 optional tag

[embray]

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

[jdemeyer]

Replying to embray:

I'm not opposed

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

[chapoton]

This seems to be ready to go, no ?

[chapoton]

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

[embray]

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

[chapoton]

follow-up in #24570