Opened 6 years ago
Last modified 5 years ago
#21292 needs_work enhancement
Add "ignored" flag for doctests
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-7.4 |
Component: | doctest framework | Keywords: | |
Cc: | leif | Merged in: | |
Authors: | Erik Bray | Reviewers: | Julian Rüth |
Report Upstream: | N/A | Work issues: | coverage |
Branch: | u/embray/doctest-ignored (Commits, GitHub, GitLab) | Commit: | 0a30f654be2dd56805d0d671e5478b93d1a68e60 |
Dependencies: | Stopgaps: |
Description
This changed the output checker to add an # ignored
flag that can be used in doctests to run a line of a test but not check its output. Actually this is just a renaming of the existing # random
flag, without the semantic weight of calling a result "random". "random" is still kept as an alias for "ignored" both for backwards compatibility, and because it could be useful as a semantic indication that some output truly is expected to be random.
This is useful for cases like the one I described here, where for documentation purposes it's nice to display the output of some code, but that output may not be reliable across platforms, and ultimately does not affect the result of the test.
Change History (15)
comment:1 Changed 6 years ago by
- Branch set to u/embray/doctest-ignored
- Commit set to 0a30f654be2dd56805d0d671e5478b93d1a68e60
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Cc leif added
comment:3 Changed 6 years ago by
In the description of ignored
, I'd elaborate the use case you give (which isn't that obvious).
(Just about one sentence I'd say.)
Why did you add ?:
to the ignored_marker
regex???
comment:4 follow-up: ↓ 5 Changed 6 years ago by
Yeah, I could add a little more context to the example (it's a simplified version of the test that caused me trouble in the first place).
The ?:
is just force of habit--I like to put it in ()
if I'm not actually planning to use the term in the parens as a match group. It's harmless either way.
comment:5 in reply to: ↑ 4 Changed 6 years ago by
Replying to embray:
The
?:
is just force of habit--I like to put it in()
if I'm not actually planning to use the term in the parens as a match group. It's harmless either way.
Ah, I see. So we won't (immediately) know whether ignored
or random
was specified if the pattern for these two markers matches, but don't care either, at least at the moment. That's ok.
(The other regexs aren't all optimal, e.g. regarding optional
or parsing numbers in scientific notation, but that's for another ticket.)
comment:6 Changed 6 years ago by
- Reviewers set to Julian Rüth
- Status changed from needs_review to needs_work
- Work issues set to coverage
Looks good but the patchbot complains that "random" has no doctest. I guess you should add one though it is somewhat artificial.
comment:7 Changed 5 years ago by
Revisiting this... To be honest, I never really liked the idea of having two different doctest markers doing the same thing. Wouldn't it be sufficient to use # random
with additional comments to explain why it's random?
sage: 6*9 # random (depending on default number base) 42 sage: moon.phase # random (depending on the phase of the moon) 'full'
comment:8 follow-up: ↓ 9 Changed 5 years ago by
The idea was to have #ignored
eventually replace #random
since the latter is poorly-named. It's useful to be able to skip individual doctests that are not "random".
comment:9 in reply to: ↑ 8 Changed 5 years ago by
Replying to embray:
The idea was to have
#ignored
eventually replace#random
since the latter is poorly-named.
I don't care much (it's bikeshedding anyway). However, if you do change # random
to # ignored
, I would prefer to actually make the changes in all doctests (not so hard using a regex) right now instead of "eventually". Having some doctests with # random
and some with # ignored
is confusing IMHO.
comment:10 follow-up: ↓ 11 Changed 5 years ago by
I don't know--I think it's useful to be able to say "this output is random". Depends on the case.
comment:11 in reply to: ↑ 10 Changed 5 years ago by
Replying to embray:
I don't know--I think it's useful to be able to say "this output is random". Depends on the case.
I don't see much semantic difference between "random" and "ignored": both indicate that the output of the doctest is not always the same (but the test should be run to check that it does not raise exceptions). I would say: pick one and consistently use it.
comment:12 follow-up: ↓ 14 Changed 5 years ago by
Alright, I was just trying to preserve the original purpose of the flag, which I thought made some sense insofar as specifying that the output should be truly random, whereas there are other reasons to ignore the output of a test (maybe it's not reliably deterministic, but not random). Maybe having a separate #random
would make more sense if it somehow checked that the output of that test were actually different between each run. But unless anyone actually needs that I'd be fine with getting rid of it entirely then.
comment:13 Changed 5 years ago by
On second thought, there's so many cases of # random
in the source code that if I try to change it it may conflict with just about every other unmerged branch.
Perhaps it should suffice to just update the documentation to suggest using # ignored
instead of # random
and keep the latter deprecated.
comment:14 in reply to: ↑ 12 Changed 5 years ago by
Replying to embray:
maybe it's not reliably deterministic, but not random
And what is the semantic difference between "not reliably deterministic" and "random"? I think you are trying to see a difference where there is none.
Again, I'm not strictly against this ticket, I just wouldn't bother...
comment:15 Changed 5 years ago by
In some cases it might not be random. For example, you might have a test that prints a dictionary, and the key order may be always the same on a particular Python version on a particular platform. But that doesn't mean it should be relied on to be ordered properly.
Or you might have a test that prints out a filesystem path that obviously won't be the same on all machines. That doesn't make it random. I just think random is a misnomer for this purpose. I don't think we're disagreement. You just say you wouldn't bother--but I already did because it bothered me. The only issue here is what to do with the existing name. I may have implemented something else but I don't think it's worth it to go changing the existing usage everywhere either :)
New commits:
Rename the 'random' marker to the more general 'ignored'.