Opened 4 years ago
Closed 4 years ago
#26110 closed defect (fixed)
Tests marked "optional - dochtml" are not run by default
Reported by: | jhpalmieri | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-8.4 |
Component: | doctest framework | Keywords: | |
Cc: | jdemeyer | Merged in: | |
Authors: | Jeroen Demeyer | Reviewers: | John Palmieri |
Report Upstream: | N/A | Work issues: | |
Branch: | 6a50903 (Commits, GitHub, GitLab) | Commit: | 6a5090389bde02be3498fb05e9b18834de8eac52 |
Dependencies: | #26184 | Stopgaps: |
Description
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.
Change History (47)
comment:1 Changed 4 years ago by
- Branch set to u/jhpalmieri/run-dochtml-tests-by-default
comment:2 Changed 4 years ago by
- Commit set to 8cff1a67231a1ac604d9daa5637793b0fd406f17
- Status changed from new to needs_review
comment:3 Changed 4 years ago by
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.
comment:4 Changed 4 years ago by
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.
comment:5 Changed 4 years ago by
- Commit changed from 8cff1a67231a1ac604d9daa5637793b0fd406f17 to 78b3df8b94cd36d2e907bb00b3d467c9bc2bd1d5
Branch pushed to git repo; I updated commit sha1. New commits:
78b3df8 | trac 26110: the make targets 'testall', 'testoptional', etc., should
|
comment:6 Changed 4 years ago by
- Cc jdemeyer added
comment:7 Changed 4 years ago by
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.
comment:8 Changed 4 years ago by
- Branch changed from u/jhpalmieri/run-dochtml-tests-by-default to u/jdemeyer/run-dochtml-tests-by-default
comment:9 Changed 4 years ago by
- Commit changed from 78b3df8b94cd36d2e907bb00b3d467c9bc2bd1d5 to d6d4f0643130d054edd8265bd724207a3b1cce61
comment:10 Changed 4 years ago by
- Reviewers set to Jeroen Demeyer
comment:11 Changed 4 years ago by
-
Makefile
diff --git a/Makefile b/Makefile index d81d450..1f83cbb 100644
a b micro_release: bdist-clean sagelib-clean 111 111 TESTALL = ./sage -t --all 112 112 PTESTALL = ./sage -t -p --all 113 113 114 # Flags for ./sage -t --all: 115 OPTIONAL_AND_EXTERNAL = --optional=sage,dochtml,optional,external 116
Why is this called something so specific as "OPTIONAL_AND_EXTERNAL" and not just "TESTALL_FLAGS" or something?
comment:12 Changed 4 years ago by
- Branch changed from u/jdemeyer/run-dochtml-tests-by-default to u/jhpalmieri/run-dochtml-tests-by-default
comment:13 Changed 4 years ago by
- Commit changed from d6d4f0643130d054edd8265bd724207a3b1cce61 to 6a5090389bde02be3498fb05e9b18834de8eac52
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
|
comment:14 follow-up: ↓ 16 Changed 4 years ago by
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.
comment:15 follow-ups: ↓ 18 ↓ 24 Changed 4 years ago by
Replying to jdemeyer in #25345:
Replying to gh-timokau:
I also make sure that the optional
py2
tag is only added ifsage
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.
comment:16 in reply to: ↑ 14 Changed 4 years ago by
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.
comment:17 Changed 4 years ago by
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.
comment:18 in reply to: ↑ 15 ; follow-up: ↓ 19 Changed 4 years ago by
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 ifsage
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 thesage
tests being run too. They need the context (variables being set etc.). Thepy2
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 aspy2
, 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
comment:19 in reply to: ↑ 18 Changed 4 years ago by
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 insage/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.
comment:20 Changed 4 years ago by
John: do you agree with my patch from 9?
comment:21 Changed 4 years ago by
- Reviewers changed from Jeroen Demeyer to John Palmieri
- Status changed from needs_review to positive_review
Yes, I agree with it, plus my added documentation. Positive review from me.
comment:22 follow-up: ↓ 27 Changed 4 years ago by
- Status changed from positive_review to needs_work
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.
comment:23 Changed 4 years ago by
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?
comment:24 in reply to: ↑ 15 Changed 4 years ago by
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.
comment:25 follow-up: ↓ 26 Changed 4 years ago by
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
).
comment:26 in reply to: ↑ 25 Changed 4 years ago by
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. Runningsage -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.
comment:27 in reply to: ↑ 22 ; follow-up: ↓ 28 Changed 4 years ago by
- Status changed from needs_work to positive_review
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.
comment:28 in reply to: ↑ 27 ; follow-up: ↓ 30 Changed 4 years ago by
- Status changed from positive_review to needs_work
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 treatspy2
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.
comment:29 Changed 4 years ago by
I've created #26159 as a sort of "wishlist" ticket for a general solution. I won't try to implement that myself though.
comment:30 in reply to: ↑ 28 ; follow-up: ↓ 32 Changed 4 years ago by
- Status changed from needs_work to positive_review
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.
comment:31 follow-up: ↓ 33 Changed 4 years ago by
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.
comment:32 in reply to: ↑ 30 ; follow-up: ↓ 37 Changed 4 years ago by
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
andsage
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
andsage
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.
comment:33 in reply to: ↑ 31 Changed 4 years ago by
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 skippy2
tests unlesssage
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.
comment:34 follow-up: ↓ 35 Changed 4 years ago by
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.
comment:35 in reply to: ↑ 34 ; follow-up: ↓ 36 Changed 4 years ago by
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
andpy3
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.
comment:36 in reply to: ↑ 35 ; follow-up: ↓ 38 Changed 4 years ago by
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.
comment:37 in reply to: ↑ 32 Changed 4 years ago by
Can we please move the discussion about py2
to a different ticket? I'm totally open for discussions, but not on this ticket.
comment:38 in reply to: ↑ 36 Changed 4 years ago by
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.
comment:39 Changed 4 years ago by
- Status changed from positive_review to needs_work
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 ----------------------------------------------------------------------
comment:40 Changed 4 years ago by
You must have merged #26017 to get this failure.
comment:41 Changed 4 years ago by
When I merge #26017 with this ticket, I don't get any failures with make ptestlong
. Volker, can you clarify?
comment:42 Changed 4 years ago by
- Status changed from needs_work to positive_review
I agree: the doctests which are failing are doctests added by #26017
comment:43 follow-up: ↓ 45 Changed 4 years ago by
- Status changed from positive_review to needs_work
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.
comment:44 Changed 4 years ago by
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.
comment:45 in reply to: ↑ 43 Changed 4 years ago by
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.
comment:46 Changed 4 years ago by
- Dependencies set to #26184
- Status changed from needs_work to positive_review
comment:47 Changed 4 years ago by
- Branch changed from u/jhpalmieri/run-dochtml-tests-by-default to 6a5090389bde02be3498fb05e9b18834de8eac52
- Resolution set to fixed
- Status changed from positive_review to closed
I think this will take care of it, but someone who knows the doctesting framework should review this (and #25345) carefully.
New commits:
trac 26110: by default, always run tests marked "optional - dochtml".