Tests marked "optional - dochtml" are not run by default

[jhpalmieri]

According to #25345, tests marked optional - dochtml are supposed to be run by default, but they are not, at least not for me. When I run doctests, I see

Using --optional=coxeter3,mpir,python2,sage

Note that dochtml is missing. As an experiment, I added

    sage: 1 + 1 == 3 # optional -- dochtml
    True

to a file, and when I ran sage -t ... on that file, tests passed. (Tests failed if I included the --optional=dochtml flag.)

Marked as a blocker because as things stand, we are skipping tests that we should not be skipping, and that we were not skipping before #25345.

[jhpalmieri]

I think this will take care of it, but someone who knows the doctesting framework should review this (and #25345) carefully.

---

New commits:

​8cff1a6

trac 26110: by default, always run tests marked "optional - dochtml".

[embray]

I think this was added just recently, and the intent was explicitly to not require these tests by default.

I argued at the time that maybe it should be enabled by default, but only if SAGE_DOC can be found.

[jhpalmieri]

Recent, yes: merged in 8.4.beta1. But the intention was to keep testing by default:

• From the description at #25345: "This patch makes the doctests that depend on the htmldocs optional, but enables those doctests by default."
• From ​comment 23 there: "This change does not disable documentation in any way. It is still enabled by default and tested by default."

There would have been serious objections by me and by Jeroen (see ​his comment 24) to that ticket if it changed the default testing behavior.

[git]

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

​78b3df8

trac 26110: the make targets 'testall', 'testoptional', etc., should

[jdemeyer]

I'm sorry but I don't like this solution. The duplication between sage-runtests script and the doctest framework is bad: it violates "Don't Repeat Yourself (DRY)" leading to errors such as this ticket.

[git]

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

​e75f346	Don't specify --optional for the default optional tags
​d6d4f06	Clarify defaults

[embray]

Makefile

@@ -111 +111 @@
 TESTALL = ./sage -t --all
 PTESTALL = ./sage -t -p --all
 
+# Flags for ./sage -t --all:
+OPTIONAL_AND_EXTERNAL = --optional=sage,dochtml,optional,external
+

Why is this called something so specific as "OPTIONAL_AND_EXTERNAL" and not just "TESTALL_FLAGS" or something?

[jhpalmieri]

I changed "OPTIONAL_AND_EXTERNAL" to "TESTALL_FLAGS". I also added a little documentation (which I should have done in my initial version).

---

New commits:

​6a50903

trac 26110: a little documentation in Makefile, sage-runtests

[gh-timokau]

Yes disabling doctest framework testing by default was definitely *not* my intention. Sorry that it turned out that way (hard to test the testing).

That said I agree with Jeroen. You wrote a comment

# NOTE that these are NOT the defaults used by the sage-runtests # script (which is what gets invoked when running sage -t). # These are only basic defaults when invoking the doctest runner # from Python, which is not the typical use case.

Is there any reason it should work that way? Refactoring that could be done separately from this ticket though if it would take too much time. Fixing the default doctests has higher priority than refactoring.

[gh-timokau]

Replying to ​jdemeyer in #25345:

Replying to gh-timokau:

I also make sure that the optional py2 tag is only added if sage is in the optionals list.

This change doesn't really make sense to me. I think it's a feature that # py2 tests are always run on Python 2, regardless of the --optional option. So I'm reverting this part in #26110.

The reason I did that change is because the py2 tests were not able to be run without the sage tests being run too. They need the context (variables being set etc.). The py2 tag was very much used as if it was intended to be used in an "and" combination with sage, not "or".

If that is not the case and we're reverting that, we need to mark the context of py2 tests as py2, too.

[jhpalmieri]

Replying to gh-timokau:

Yes disabling doctest framework testing by default was definitely *not* my intention. Sorry that it turned out that way (hard to test the testing).

That said I agree with Jeroen. You wrote a comment

# NOTE that these are NOT the defaults used by the sage-runtests # script (which is what gets invoked when running sage -t). # These are only basic defaults when invoking the doctest runner # from Python, which is not the typical use case.

Is there any reason it should work that way? Refactoring that could be done separately from this ticket though if it would take too much time. Fixing the default doctests has higher priority than refactoring.

Jeroen wrote the comment, but my reply would be: it is easy to figure out that sage-runtests runs the tests, and once you know that, it is pretty easy to find out what flags it uses. If you put the flags in doctest/control.py, they are much harder to find.

[gh-timokau]

It still seems confusing to me to split the doctest options over two files with a little redundancy. But at least with the comment nobody should make the same mistake I did, so maybe that's good enough.

[jhpalmieri]

Replying to gh-timokau:

Replying to ​jdemeyer in #25345:

Replying to gh-timokau:

I also make sure that the optional py2 tag is only added if sage is in the optionals list.

This change doesn't really make sense to me. I think it's a feature that # py2 tests are always run on Python 2, regardless of the --optional option. So I'm reverting this part in #26110.

The reason I did that change is because the py2 tests were not able to be run without the sage tests being run too. They need the context (variables being set etc.). The py2 tag was very much used as if it was intended to be used in an "and" combination with sage, not "or".

If that is not the case and we're reverting that, we need to mark the context of py2 tests as py2, too.

There are no guidelines in the Sage documentation about this, as far as I know, and there are certainly doctests in the Sage library in which the "context" is not marked at all, and only the line actually using the optional part is marked as optional. (This is done in sage/misc/latex.py, for example, and also in a few files in sage/homology.) You may very well want to illustrate some basic computations, and then, if an optional package happens to be present, also do something fancier, but the basic parts should always be doctested. It would be redundant and awkward to have

sage: basic computation 1
sage: basic computation 2
sage: basic computation 3

sage: basic computation 1 # optional - fancy
sage: basic computation 2 # optional - fancy
sage: basic computation 3 # optional - fancy
sage: fancy computation   # optional - fancy

[gh-timokau]

Replying to jhpalmieri:

There are no guidelines in the Sage documentation about this, as far as I know, and there are certainly doctests in the Sage library in which the "context" is not marked at all, and only the line actually using the optional part is marked as optional. (This is done in sage/misc/latex.py, for example, and also in a few files in sage/homology.) You may very well want to illustrate some basic computations, and then, if an optional package happens to be present, also do something fancier, but the basic parts should always be doctested. It would be redundant and awkward to have

Yes this was my understanding too, which is why I only added py2 conditional on the "basic parts" (optional - sage) being tested too. A better solution would be to be able to express "run this when sage AND py2 is present" vs "run this when sage OR py2 is present", but that would be a significant change in the doctesting framework.

[jdemeyer]

John: do you agree with my patch from 9?

[jhpalmieri]

Yes, I agree with it, plus my added documentation. Positive review from me.

[gh-timokau]

As I said before, adding py2 unconditionally makes it impossible to run the dochtml doctests in isolation. I think that should be fixed either by reverting that change, modifying the doctest framework to make "OR" possible or giving the py2 tests their necessary context.

[jhpalmieri]

I don't understand: where have you talked about py2 tests and dochtml tests? Or did you mean py2 and sage? Anyway, can you give an example illustrating the issue?

[gh-timokau]

This is what I was talking about here and here. The point of the orignal ticket that introduced this bug was to be able to build and test sage and its docs seperately. When py2 is always added by default, the docs cannot be tested individually like this:

sage -t --optional=dochtml --all

Because the py2 tests will be run without their context. That is why I originally made the change to only add py2 when sage is also added (since the py2 tests assume that all unmarked doctests are also run), which Jeroen reverted here.

[jhpalmieri]

The point of the orignal ticket that introduced this bug was to be able to build and test sage and its docs seperately.

The stated point of the original ticket was to be able to test the Sage library and skip over tests which depended on the docs; this is not the same as being able to test the docs independently, and if that was one of the goals of #25345, it was not clear to me. Given the situation with other optional tags in Sage, you shouldn't expect sage -t --optional=dochtml --all to pass. Running sage -t --optional=latex --all results in some failed tests, for example. Same with --optional=chomp.

As I said before, there are no guidelines that optional tests marked "TAG" must pass when run with only --optional=TAG. Therefore we shouldn't start imposing that restriction without an explicit agreement that the policy is being changed. Right now, the de facto policy is: optional tests marked "TAG" should pass when run with --optional=sage,TAG (but they don't need to pass with --optional=TAG).

[gh-timokau]

Replying to jhpalmieri:

The point of the orignal ticket that introduced this bug was to be able to build and test sage and its docs seperately.

The stated point of the original ticket was to be able to test the Sage library and skip over tests which depended on the docs; this is not the same as being able to test the docs independently, and if that was one of the goals of #25345, it was not clear to me.

You're right, I probably failed to say that explicitly. My reasoning is that I want to build the docs seperately from the rest of sage but I would still like to run those tests. That was possible before.

Given the situation with other optional tags in Sage, you shouldn't expect sage -t --optional=dochtml --all to pass. Running sage -t --optional=latex --all results in some failed tests, for example. Same with --optional=chomp.

But it did pass since the dochtml doctests are few and pretty isolated.

As I said before, there are no guidelines that optional tests marked "TAG" must pass when run with only --optional=TAG. Therefore we shouldn't start imposing that restriction without an explicit agreement that the policy is being changed. Right now, the de facto policy is: optional tests marked "TAG" should pass when run with --optional=sage,TAG (but they don't need to pass with --optional=TAG).

I agree that a general discussion is out of scope (although I think that is a problem and should be solved some day). But this is of a much more limited scope and easy to solve. There are also no downsides to only adding py2 when sage is added since it won't work otherwise anyways.

[jdemeyer]

Replying to gh-timokau:

I think that should be fixed either by reverting that change, modifying the doctest framework to make "OR" possible or giving the py2 tests their necessary context.

That issue is really unrelated to this ticket, so let's not confound two issues.

I still think that the handling of py2 in this ticket is better than before: it treats py2 like all other optional tags, making things more consistent (agreed, consistently "wrong" but consistence is good).

I'm fine with having a wider discussion about the meaning of --optional=sage but that shouldn't hold up this ticket.

[gh-timokau]

Replying to jdemeyer:

Replying to gh-timokau:

I think that should be fixed either by reverting that change, modifying the doctest framework to make "OR" possible or giving the py2 tests their necessary context.

That issue is really unrelated to this ticket, so let's not confound two issues.

It worked before this ticket and doesn't work after. So I think it is related.

I still think that the handling of py2 in this ticket is better than before: it treats py2 like all other optional tags, making things more consistent (agreed, consistently "wrong" but consistence is good).

No, py2 is differnt from other optional tags because it is added automatically. Before it was added automatically only when it made sense. With this change it is added in any case. I don't think that is an improvement. I'll set this back to needs_work once more because I really think it does. The issue is relevant to this ticket, easy to fix and with no disadvantage. If you want to set it to positive_review again, I'll have to accept that and just not run the dochtml tests at all for now.

[gh-timokau]

I've created #26159 as a sort of "wishlist" ticket for a general solution. I won't try to implement that myself though.

[jdemeyer]

Replying to gh-timokau:

Before it was added automatically only when it made sense.

This is where I do not agree. py2 and sage are semantically completely unrelated. The only reason why you think that they are related is to work around a bug in the doctester. I say: don't work around that bug but fix that bug. I see that you just opened #26159 for that. Once you fix that, you will realize that py2 and sage have nothing to do with eachother.

The only part where I do agree is that I shouldn't be changing anything to py2 on this ticket. But you also shouldn't have changed it on #25345 either. So let's just revert this to how it was in Sage 8.3 and discuss it in #26159. I honestly think that it's the most sensible thing to do.

[jhpalmieri]

I"m not sure what the bug in the doctester is, but since #26159 is described as a wishlist, I think it is less focused then just being aimed at a single issue involving py2. So I think a more focused ticket should be opened. I would do it, but I'm not sure what the issue is. Is it that we want sage -t --optional=tag1,tag2,... to skip py2 tests unless sage is one of the tags? I can see some sense in that: we have in the Sage library tests like this:

sage: setup
sage: setup
sage: test something # py2
ANSWER
sage: test something # py3
DIFFERENT ANSWER

and I don't want to have to mark the first two lines with # py2, py3. With --optional=dochtml, only the third line will get executed, resulting in a failed test. Is that the point?

It's still not obvious to me that it's a bug, and I think it belongs on a separate ticket, but I certainly don't mind if it gets fixed.

[gh-timokau]

Replying to jdemeyer:

Replying to gh-timokau:

Before it was added automatically only when it made sense.

This is where I do not agree. py2 and sage are semantically completely unrelated.

Maybe not semantically but in practice they are. I don't see why we should hinder a practical usecase for that. Currently all py2 tests assume that sage tests are run so it doesn't make any sense to add py2 when sage is not tested. It does not have any advantage whatsoever to add py2 when sage is not added while it does have an advantage not to add it.

The only reason why you think that they are related is to work around a bug in the doctester. I say: don't work around that bug but fix that bug. I see that you just opened #26159 for that. Once you fix that, you will realize that py2 and sage have nothing to do with eachother.

I don't think we should introduce a regression that can be easily fixed and does not have any disadvantages on typical workflow just because the solution is not good enough yet. A better solution can still come in the future but I expect that to take significant changes in the doctesting framework.

The only part where I do agree is that I shouldn't be changing anything to py2 on this ticket. But you also shouldn't have changed it on #25345 either.

I changed in in #25345 to make the use case of testing sage and dochtml seperately possible. I just never explicitly said that I intended to also run the dochtml tests. But I still think that was in-scope for that ticket.

So let's just revert this to how it was in Sage 8.3 and discuss it in #26159. I honestly think that it's the most sensible thing to do.

I disagree but apparently your mind is made up.

[gh-timokau]

Replying to jhpalmieri:

I"m not sure what the bug in the doctester is

It may be a bit much to call it a bug but I think Jeroen is talking about the inadequacy that currently makes it necessary to always include sage in the optionals list.

but since #26159 is described as a wishlist, I think it is less focused then just being aimed at a single issue involving py2. So I think a more focused ticket should be opened.

The current behaviour is the one I want and introduced in #25345 (kind of out of scope because I did not mention that use case there). The change is revertet in this ticket:

-                if "sage" in options.optional:
-                    options.optional |= auto_optional_tags
+                options.optional |= auto_optional_tags

So there would be little use in reverting it here and then re-introducing it in another ticket. I think it shouldn't be reverted in the first place.

I would do it, but I'm not sure what the issue is. Is it that we want sage -t --optional=tag1,tag2,... to skip py2 tests unless sage is one of the tags?

We currently add some --optional tags by default. When we detect python2 we add the py2 tag. In #25345 I changed that so that py2 is only added when sage is also tested. Here that change is reverted.

The use case is that I first build sage without doctest, testing sage -t --optional=sage. Then I have a different package which depends on sage, build the docs and is tested by sage -t --optional=dochtml. Before #25345 (and after this is merged) that will not work because it will also automatically try to run all the py2 tests wihtout context.

[jhpalmieri]

So among the issues is your particular use case, which happened to work after #25345, but is not guaranteed to work in the future because someone might add dochtml flags without marking their context. This is not very compelling to me.

A separate issue is the particular handling of the py2 and py3 tags. That could go on another ticket.

[gh-timokau]

Replying to jhpalmieri:

So among the issues is your particular use case, which happened to work after #25345, but is not guaranteed to work in the future because someone might add dochtml flags without marking their context. This is not very compelling to me.

In theory yes, that might happen. In practice there currently only are 16 lines marked as optional - dochtml and all of them are isolated enough to work. I think it is pretty unlikely that more will be added any time soon (since not many tests actually require the docs to be build) and even more unlikely that those will need context.

So it is a question of definitely breaking it now vs. it hypothetically maybe one day stop working.

A separate issue is the particular handling of the py2 and py3 tags. That could go on another ticket.

Do you mean that they are always added by default? They do need to be handled somewhat special because they need to determine the python version at runtime.

[jhpalmieri]

Replying to gh-timokau:

Do you mean that they are always added by default? They do need to be handled somewhat special because they need to determine the python version at runtime.

I mean whether they should be treated differently (only added when sage is also tested) or treated like the optional tags.

[jdemeyer]

Can we please move the discussion about py2 to a different ticket? I'm totally open for discussions, but not on this ticket.

[gh-timokau]

Replying to jhpalmieri:

Replying to gh-timokau:

Do you mean that they are always added by default? They do need to be handled somewhat special because they need to determine the python version at runtime.

I mean whether they should be treated differently (only added when sage is also tested) or treated like the optional tags.

Right. At least moving option (1) into another ticket doesn't make much sense to me though because that is the status quo that is changed by this ticket.

[vbraun]

Doctesting 1 file.
sage -t --long src/sage/misc/sagedoc.py
**********************************************************************
File "src/sage/misc/sagedoc.py", line 1179, in sage.misc.sagedoc.?
Failed example:
    len(L) < N  # optional - dochtml, long time
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage/misc/sagedoc.py", line 1183, in sage.misc.sagedoc.?
Failed example:
    all(tree_re.search(l) for l in L) # optional - dochtml, long time
Expected:
    True
Got:
    False
**********************************************************************
1 item had failures:
   2 of  35 in sage.misc.sagedoc.?
    [115 tests, 2 failures, 49.04 s]
----------------------------------------------------------------------
sage -t --long src/sage/misc/sagedoc.py  # 2 doctests failed
----------------------------------------------------------------------

[jhpalmieri]

You must have merged #26017 to get this failure.

[jhpalmieri]

When I merge #26017 with this ticket, I don't get any failures with make ptestlong. Volker, can you clarify?

[jdemeyer]

I agree: the doctests which are failing are doctests added by #26017

[vbraun]

Guys, you were supposed to wait for the next beta and not throw a wrench in the release management process by setting a ticket that does not work back to positive review.

[jhpalmieri]

Volker, could you please say something like "doctest failure in upcoming beta" rather than just post a failure with no explanation? Sometimes there are failures purely caused by the ticket that the authors and reviewers missed, and those probably deserve no explanation, but otherwise it could help if we know the context for the failures.

[jdemeyer]

Replying to vbraun:

Guys, you were supposed to wait for the next beta and not throw a wrench in the release management process by setting a ticket that does not work back to positive review.

At the time of setting it to positive review, it passed all doctests. We mentioned that the failures were really because of #26017 but you ignored that. Now that #26017 is merged, this ticket indeed breaks.

In other words: I don't think that we (on this ticket) did anything wrong.