Bug 78750 – NRWT does not report failure for a skipped test

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alexey Proskuryakov]

fast/files/apply-blob-url-to-img.html fails in WebKit2, because eventSender.beginDragWithFiles is simply not implemented. But run-webkit-test -2 fast/files/apply-blob-url-to-img.html says:

run-webkit-tests fast/files/apply-blob-url-to-img.html -2   
All 1 tests ran as expected.

This is not helpful - I told NRWT to skip the test when running them all, but not to lie to me when the test is forced to run like this.

[Dirk Pranke]

The problem is that the test is in the Skipped file, and we have code that assumes that all tests that are in Skipped files are expected to FAIL . So, your test is failing, which means that it is running "as expected".

I could see how this would be confusing, and it'd probably be better if we marked them as expected to PASS, so that when we do --force the test, it is reported as failing by default.

[Adam Barth]

Shouldn't we assume that all tests in the Skipped file are marked SKIP ?

[Dirk Pranke]

probably. 'SKIP' isn't (or at least didn't used to be) a valid (right-hand-side) expectation for a test. If that is now legal, it would be better. I was thinking that some expectation like "unknown" would be ideal, since we don't actually know what the expected result is for tests in a skipped file; SKIP would work fine for that.

[Adam Barth]

Ah, I see.  Yeah, PASS is probably a good default then.

[Dirk Pranke]

Created attachment 127307 [details]
Patch

[Eric Seidel (no email)]

Dirk: So what would the new output be for ap, after your change?

[Dirk Pranke]

Good question :)

$ run-webkit-tests --force --platform test --no-new-test-results --no-retry-failures \
failures/expected/skip_text.html failures/unexpected/skip_pass.html
  failures/expected/skip_text.html -> unexpected text diff mismatch
  failures/unexpected/skip_pass.html -> unexpected pass
0 tests ran as expected, 2 didn't:


Expected to skip, but passed: (1)
  failures/unexpected/skip_pass.html


Regressions: Unexpected text diff mismatch : (1)
  failures/expected/skip_text.html = TEXT


MOCK: user.open_url: file://...
$

Note the failing test doesn't say "Expected to skip, but failed with a text diff mismatch"; there is no indication that it was marked as SKIP ... I can probably make something like that happen if there is a desire, but that will be a more invasive change, I suspect.

[Alexey Proskuryakov]

What's the output with the original proposed fix (where being in Skipped file means "pass")? IIRC, it was better.

[Dirk Pranke]

(In reply to comment #8)
> What's the output with the original proposed fix (where being in Skipped file means "pass")? IIRC, it was better.

$ new-run-webkit-tests --force --no-new-test-results --no-retry-failures --platform test failures/unexpected/skip_pass.html failures/expected/skip_text.html
  failures/expected/skip_text.html -> unexpected text diff mismatch
1 test ran as expected, 1 didn't:


Regressions: Unexpected text diff mismatch : (1)
  failures/expected/skip_text.html = TEXT


MOCK: user.open_url: file://...
layout_tests $

You get the message about the test that fails, but not the test that passes.

I think the first example (and hence the patch that I uploaded) is better, but I don't feel strongly about it; if you do, I can drop the code that prints the unexpected passes.

[Alexey Proskuryakov]

For my purposes, the simple fix (with output as in comment 9) works perfectly.

[Dirk Pranke]

Eric, Adam, Tony, Ojan, any of you have any leanings?

[Dirk Pranke]

Created attachment 127647 [details]
argh ... ignore; wrong bug.

[Dirk Pranke]

*** Bug 78931 has been marked as a duplicate of this bug. ***

[Ojan Vafai]

I prefer the less verbose output of comment 9. In general, the output when there are failures is still too verbose IMO. Anything we can do to reduce it is better. In this case, the 'Expected to skip, but passed' doesn't really provide any actionable information IMO.

[Dirk Pranke]

(In reply to comment #14)
> I prefer the less verbose output of comment 9.

Alrighty then. Two votes for terseness is enough for me.

> In general, the output when there are failures is still too verbose IMO. 

Ojan: so (independent of this change) what would you kill in the output for failures? the summary at the end? the failures as they occur? Feel free to file another bug and/or mail me off-bugzilla.

[Dirk Pranke]

Created attachment 127668 [details]
make Skipped tests expected to PASS instead of SKIP

[Eric Seidel (no email)]

Comment on attachment 127668 [details]
make Skipped tests expected to PASS instead of SKIP

OK.

[Dirk Pranke]

Committed r108143: <http://trac.webkit.org/changeset/108143>