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:

Status badges

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 embray

  • Authors set to Erik Bray
  • Branch set to u/embray/doctest-ignored
  • Commit set to 0a30f654be2dd56805d0d671e5478b93d1a68e60
  • Status changed from new to needs_review

New commits:

0a30f65Rename the 'random' marker to the more general 'ignored'.

comment:2 Changed 6 years ago by embray

  • Cc leif added

comment:3 Changed 6 years ago by leif

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: Changed 6 years ago by embray

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 leif

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 5 years ago by saraedum

  • 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 jdemeyer

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: Changed 5 years ago by embray

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 jdemeyer

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: Changed 5 years ago by embray

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 jdemeyer

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: Changed 5 years ago by embray

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 embray

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 jdemeyer

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 embray

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.

Version 0, edited 5 years ago by embray (next)
Note: See TracTickets for help on using tickets.