Bug 78750 - NRWT does not report failure for a skipped test
Summary: NRWT does not report failure for a skipped test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Critical
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-15 14:59 PST by Alexey Proskuryakov
Modified: 2012-02-17 16:56 PST (History)
6 users (show)

See Also:


Attachments
Patch (7.67 KB, patch)
2012-02-15 20:51 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
argh ... ignore; wrong bug. (3.27 KB, patch)
2012-02-17 13:48 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
make Skipped tests expected to PASS instead of SKIP (5.72 KB, patch)
2012-02-17 15:54 PST, Dirk Pranke
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2012-02-15 14:59:01 PST
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.
Comment 1 Dirk Pranke 2012-02-15 15:17:12 PST
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.
Comment 2 Adam Barth 2012-02-15 15:33:38 PST
Shouldn't we assume that all tests in the Skipped file are marked SKIP ?
Comment 3 Dirk Pranke 2012-02-15 15:38:29 PST
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.
Comment 4 Adam Barth 2012-02-15 15:46:09 PST
Ah, I see.  Yeah, PASS is probably a good default then.
Comment 5 Dirk Pranke 2012-02-15 20:51:32 PST
Created attachment 127307 [details]
Patch
Comment 6 Eric Seidel (no email) 2012-02-16 15:08:01 PST
Dirk: So what would the new output be for ap, after your change?
Comment 7 Dirk Pranke 2012-02-16 19:59:37 PST
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.
Comment 8 Alexey Proskuryakov 2012-02-16 23:45:45 PST
What's the output with the original proposed fix (where being in Skipped file means "pass")? IIRC, it was better.
Comment 9 Dirk Pranke 2012-02-17 13:32:12 PST
(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.
Comment 10 Alexey Proskuryakov 2012-02-17 13:35:57 PST
For my purposes, the simple fix (with output as in comment 9) works perfectly.
Comment 11 Dirk Pranke 2012-02-17 13:42:37 PST
Eric, Adam, Tony, Ojan, any of you have any leanings?
Comment 12 Dirk Pranke 2012-02-17 13:48:55 PST
Created attachment 127647 [details]
argh ... ignore; wrong bug.
Comment 13 Dirk Pranke 2012-02-17 13:49:03 PST
*** Bug 78931 has been marked as a duplicate of this bug. ***
Comment 14 Ojan Vafai 2012-02-17 14:48:54 PST
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.
Comment 15 Dirk Pranke 2012-02-17 15:15:00 PST
(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.
Comment 16 Dirk Pranke 2012-02-17 15:54:16 PST
Created attachment 127668 [details]
make Skipped tests expected to PASS instead of SKIP
Comment 17 Eric Seidel (no email) 2012-02-17 16:17:13 PST
Comment on attachment 127668 [details]
make Skipped tests expected to PASS instead of SKIP

OK.
Comment 18 Dirk Pranke 2012-02-17 16:56:14 PST
Committed r108143: <http://trac.webkit.org/changeset/108143>