Opened 13 months ago

Last modified 6 months ago

#26784 needs_info enhancement

Add ability to inform the doctest runner of known failures

Reported by: embray Owned by:
Priority: major Milestone: sage-8.9
Component: doctest framework Keywords:
Cc: vbraun, chapoton Merged in:
Authors: Erik Bray Reviewers:
Report Upstream: N/A Work issues:
Branch: u/embray/doctest/known-failures (Commits) Commit: 781259b9eaf7df351f312d6f607eefe1d61efd7a
Dependencies: Stopgaps:

Description

Adds a simple feature for specifying test files that contain "known failures" i.e. they are expected to fail, so if they do the entire test run should not be counted as a failure (that is, it should still return 0). This is part of a more flexible alternative to #26740.

When tests fail in a file marked as a known-failure the only difference is that in the test result summary it is noted when failures were expected, and the test runner returns 0 unless there were other unexpected failures.

If a test marked as known-failure happens to succeed unexpected this is also noted in the summary, but is not counted as a failure.

This feature is still very simplistic: all it states is that some test file is expected to fail, not how it is expected to fail. That might be useful to add later for better specificity.

Known failures are also listed file-by-file; there is no support yet for marking whole directories as known-failures and/or wildcards. Those are also possibilities for future enhancements.

Change History (11)

comment:1 Changed 13 months ago by embray

  • Status changed from new to needs_review

comment:2 Changed 13 months ago by embray

Weird doctest failures on my docker patchbot, but I don't think they're related to this as I didn't get anything like that locally. Instead it looks like some incremental build problem on the patchbot. I may need to rebuild it.

comment:3 Changed 12 months ago by jhpalmieri

Documentation fails to build: change """ to r""" in doctest.test. Regarding

sage -t --warn-long 0.0 simple_success.rst # 4 doctests passed (known failure)

it looks to me like there are only 3 doctests in that file: the 3 lines beginning with sage:. Why does it say 4?

comment:4 Changed 12 months ago by embray

  • Milestone changed from sage-8.5 to sage-8.7

Retargeting some of my tickets (somewhat optimistically for now).

comment:5 Changed 12 months ago by embray

It says 4 because there's an automatic "implicit" doctest that checks if cysignals is in a sane state.

comment:6 Changed 11 months ago by jdemeyer

I generally don't like code like

                    try:
                        with open(filename) as fobj:
                            known_failures.update(
                                os.path.abspath(l.strip()) for l in fobj)
                    except IOError as exc:
                        raise ValueError('could not open known failures file '
                                         '"{}": {}'.format(filename, exc))

since it hides information about the original exception. I suggest to remove the try/except.

comment:7 Changed 11 months ago by jdemeyer

  • Status changed from needs_review to needs_work

I find it a bit strange that you're handing reporting of known failures both in forker.py and in reporting.py. It would seem more logical to report known failures only in reporting.py in this message:

log("    [%s, %s%.2f s]" % (total, "%s, "%(count_noun(f, "failure")) if f else "", wall))

comment:8 follow-up: Changed 11 months ago by embray

The whole point of catching an exception and raising a new one is to give a more informative exception and context around it: It will still tell you why the exception occurred "File not found" or whatever. This is for users: there's nothing really interesting in the original exception that is "hidden" that a developer can't find out with a debugger. However, I'm not sure why I even bothered in this case either, but it's harmless.

For the second comment I don't know why it "would seem more logical". Why not both? When I was working on this I found it more informative as to what was happening that way.

Last edited 11 months ago by embray (previous) (diff)

comment:9 in reply to: ↑ 8 Changed 11 months ago by embray

  • Status changed from needs_work to needs_info

Replying to embray:

The whole point of catching an exception and raising a new one is to give a more informative exception and context around it: It will still tell you why the exception occurred "File not found" or whatever. This is for users: there's nothing really interesting in the original exception that is "hidden" that a developer can't find out with a debugger. However, I'm not sure why I even bothered in this case either, but it's harmless.

Actually in this case I can see why I did it. That method already raises a ValueError if some option has an invalid value, so in this case it's treating a known_failures file that cannot be opened (for whatever reason) as an invalid value.

comment:10 Changed 9 months ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Moving all my in-progress tickets to 8.8 milestone.

comment:11 Changed 6 months ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Tickets still needing working or clarification should be moved to the next release milestone at the soonest (please feel free to revert if you think the ticket is close to being resolved).

Note: See TracTickets for help on using tickets.