Bug 62425 – new-run-webkit-tests: results.html don't list the same list of failed tests as bots do

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

[Ryosuke Niwa]

The list of failed tests on results.html don't match that of stdout in new-run-webkit-tests.  As a result, we'll have mismatch on the number of tests failed on waterfall/console view and what's shown on results.html

For example on http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/19255, 1 test failed according to stdout of new-run-webkit-tests but results.html doesn't identify the test that's causing the failure:
http://build.webkit.org/results/Chromium%20Linux%20Release%20(Tests)/r88513%20(19255)/results.html

We should fix this because it makes keeping the bots green really hard.

[Ryosuke Niwa]

Another example:
http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/15331
suggests that we have 3 test failures and 2 new passes.  But the number don't match up on
http://build.webkit.org/results/Chromium%20Win%20Release%20(Tests)/r88516%20(15331)/results.html

I think the problem comes from the fact results.html
1. does not distinguish whether a teat has crash expectation or not when listing crashed tests
2. does not distinguish whether a test has pass expectation or not when listing unexpectedly passed tests

[Dirk Pranke]

Hm. I may not have really looked at the results.html format since Ojan redid everything.

I agree the output is highly confusing. It looks like the results.html file is showing tests that crash, have MISSING expectations, etc. even if those are expected? Moreover, it does not actually tell you that any of those things are expected, nor does it distinguish those sections from the "unexpected results" section, which is empty and hence not displaying.

Frankly, I was surprised that we were generating a results.html at all when the test run is passing (i.e., there are no unexpected results), since that's not what ORWT does nor what NRWT used to do.

Ojan, are these all intended changes? Can we fix things so that it's at least clearer that the stuff that is being displayed is expected (but admittedly bad)?

I would actually argue that the *only* things that should be displayed by default are the unexpected results, and you should have to toggle the "expected crashes" into view, just like you have to toggle all of the other expected things.

Also, there may be another issue causing confusion. I'll mention it here for continuity but it probably should be a separate bug.

If I look at the chromium.webkit canary bot and find the most recent failing Linux build (which I was doing for comparison), I went from here:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux

to:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/8949

which claimed that there was 1 unexpected failure and 1 unexpectedly flaky test (which matches the stdio output). However, if I click on the layout_test_results archive link, it takes me to:

http://build.chromium.org/f/chromium/layout_test_results/Webkit_Linux/88633/

and downloading that .zip file gives me:

Desktop $ shasum layout-test-results.zip 
e86ad69aff7b2dc53e3a5cf07a4eaff287d5700e  layout-test-results.zip

assuming I'm not getting some weird caching issue from Chrome, that zip file does not show the failures listed in the stdio log at all; is it possible that the wrong layout-test-results.zip file is getting archived?

[Dirk Pranke]

Adding dimitri and koz since they're gardening and should probably be in the loop on this ...

[Ryosuke Niwa]

(In reply to comment #2)
> I would actually argue that the *only* things that should be displayed by default are the unexpected results, and you should have to toggle the "expected crashes" into view, just like you have to toggle all of the other expected things.

I agree.

> assuming I'm not getting some weird caching issue from Chrome, that zip file does not show the failures listed in the stdio log at all; is it possible that the wrong layout-test-results.zip file is getting archived?

Looking at http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/8949/steps/archive_webkit_tests_results/logs/stdio
we seems to be archiving correctly (e.g. it's archiving filesystem-uri-origin-actual.txt/expected.txt).  But you're right that zip file doesn't seem to contain what it claims to contain (e.g. results.html doesn't have any entry for filesystem-uri-origin.html and zip doesn't contain filesystem-uri-origin-actual.txt/expected.txt).

[Ryosuke Niwa]

Infrastructure bug filed as http://crbug.com/85736 for fixing zip archiving.

[Ryosuke Niwa]

Created attachment 96791 [details]
fixes the bug

[Dirk Pranke]

I took a quick look at this but it wasn't obvious to me how the patch actually would be changing what gets displayed. It sounds from the Changelog that you at least hide the expected crash/missing/timeouts by default, but how do they then get unhidden? Is it possible to clearly distinguish them from the unexpected results?

It might be helpful to post a couple of screenshots or something.

At any rate, you probably want Ojan to review this, not me, anyway.

[Ryosuke Niwa]

(In reply to comment #7)
> I took a quick look at this but it wasn't obvious to me how the patch actually would be changing what gets displayed. It sounds from the Changelog that you at least hide the expected crash/missing/timeouts by default, but how do they then get unhidden? Is it possible to clearly distinguish them from the unexpected results?

They're hidden by default because of class=expected in tbody and table.  It can be unhidden by unchecking a box in option on the upper right corner.

[Dirk Pranke]

(In reply to comment #8)
> (In reply to comment #7)
> > I took a quick look at this but it wasn't obvious to me how the patch actually would be changing what gets displayed. It sounds from the Changelog that you at least hide the expected crash/missing/timeouts by default, but how do they then get unhidden? Is it possible to clearly distinguish them from the unexpected results?
> 
> They're hidden by default because of class=expected in tbody and table.  It can be unhidden by unchecking a box in option on the upper right corner.

I'm guessing you're referring to the already existing "only show unexpected results" box? If you uncheck that, does that cause both the expected crashes/timeouts/missing *and* all of the other expected results to be displayed? It seemed like part of the reason Ojan changed the format to begin with was to be able to separate these two classes of results.

Partially my concern was that the UI didn't really distinguish expected from unexpected as well as it could, period.

[Ryosuke Niwa]

(In reply to comment #9)
> I'm guessing you're referring to the already existing "only show unexpected results" box? If you uncheck that, does that cause both the expected crashes/timeouts/missing *and* all of the other expected results to be displayed? It seemed like part of the reason Ojan changed the format to begin with was to be able to separate these two classes of results.

I talked about this with Ojan, and he told me this is a bug.

[Dirk Pranke]

(In reply to comment #10)
> (In reply to comment #9)
> > I'm guessing you're referring to the already existing "only show unexpected results" box? If you uncheck that, does that cause both the expected crashes/timeouts/missing *and* all of the other expected results to be displayed? It seemed like part of the reason Ojan changed the format to begin with was to be able to separate these two classes of results.
> 
> I talked about this with Ojan, and he told me this is a bug.

"this" == displaying expected crashes/timeouts/missing separately from other expected failures?

[Ryosuke Niwa]

(In reply to comment #11)
> "this" == displaying expected crashes/timeouts/missing separately from other expected failures?

No, the fact tests with pass expectations are showing up on the list of crashed and timeouted tests when "only show unexpected results" is checked.

[WebKit Review Bot]

Comment on attachment 96791 [details]
fixes the bug

Attachment 96791 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8831083

New failing tests:
fast/harness/results.html

[WebKit Review Bot]

Created attachment 96816 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Ryosuke Niwa]

(In reply to comment #13)
> (From update of attachment 96791 [details])
> Attachment 96791 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8831083
> 
> New failing tests:
> fast/harness/results.html

It seems like the bot didn't apply my patch properly :(  The diff is identical to what I have in my patch.

[Ojan Vafai]

Comment on attachment 96791 [details]
fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=96791&action=review

Thanks for fixing this!

> LayoutTests/fast/harness/results.html:563
> +    for (var i = 0; i < tests.length; i++) {
> +        if (!tests[i].isExpected)
> +            return true;
> +    }

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/some

Something like: return tests[i].some(function(element) { return !element.isExpected;});

> LayoutTests/fast/harness/results.html:576
>          html += '<thead><th>test</th><th>expected failure</th></thead>';

Can you add a FIXME to add the expected failure column for all the test lists if globalState().results.uses_expectations_file?

> LayoutTests/fast/harness/resources/results-test.js:435
> +    function enclosingNodeWithTagNameHasClassName(node, name, className) {

s/name/tagName

> LayoutTests/fast/harness/resources/results-test.js:436
> +        while (node && (!node.tagName || node.tagName.toLowerCase() != name))

Why not just pass in the upperCase name? That or use localName, which is lowercase.

> LayoutTests/ChangeLog:12
> +        (runTests):
> +        ():

Normally I just remove this cruft. It doesn't work well for JS code.

[Ryosuke Niwa]

Committed r88592: <http://trac.webkit.org/changeset/88592>