RESOLVED FIXED 82799
If NRWT gets killed halfway through a run, it incorrectly reports tests that weren't run as passes 
https://bugs.webkit.org/show_bug.cgi?id=82799
Summary If NRWT gets killed halfway through a run, it incorrectly reports tests that ...
Ojan Vafai
Reported 2012-03-30 15:58:11 PDT
Created attachment 134907 [details] screenshot of layout test dashboard http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win7/builds/14713/steps/webkit_tests/logs/stdio corresponds to the random green box all the way to the right in screenshot. The test wasn't run, but it shows up as a PASS on the test results dashboard. My guess is that summarize results doesn't handle tests that weren't run correctly. Although, now that I think about it, if the tests don't complete, we just shouldn't upload the results to appengine at all. Getting data for half a run is not very useful. If we really wanted to upload the data, then I think we should treat tests that weren't run however we treat skipped tests.
Attachments
screenshot of layout test dashboard (57.95 KB, image/png)
2012-03-30 15:58 PDT, Ojan Vafai 		no flags
Details
Patch (4.11 KB, patch)
2012-03-31 13:14 PDT, Ojan Vafai 		no flags
DetailsFormatted DiffDiff
Patch (8.88 KB, patch)
2012-04-10 19:24 PDT, Dirk Pranke 		ojan: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Ojan Vafai
Comment 1 2012-03-30 16:00:23 PDT
Curious, also in that screenshot is http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6/builds/14272 on the mac 10.6 bot. That ran to completion, but didn't run the fast/regions test and showed it as passing. Not sure what the issue is there, but it's definitely a bug (e.g. if we didn't run the test because it was skipped, we shouldn't show it as a PASS).
Dirk Pranke
Comment 2 2012-03-30 16:02:01 PDT
I don't actually know how we treat skipped tests ... I think treating tests that aren't run the same as SKIPs is certainly better than treating them as PASS. However, I'm wondering if we should have some other state to indicate 'skipped due to aborted run' or something.
Ojan Vafai
Comment 3 2012-03-30 16:05:00 PDT
(In reply to comment #2) > I don't actually know how we treat skipped tests ... I think treating tests that aren't run the same as SKIPs is certainly better than treating them as PASS. However, I'm wondering if we should have some other state to indicate 'skipped due to aborted run' or something. I thought about that. At first glance, it doesn't seem worth the extra complexity for such a rare occurrence.
Ojan Vafai
Comment 4 2012-03-30 16:05:29 PDT
The complexity I refer to is not the code so much as having yet another thing that people working with the dashboard need to understand.
Dirk Pranke
Comment 5 2012-03-30 16:07:12 PDT
(In reply to comment #3) > (In reply to comment #2) > > I don't actually know how we treat skipped tests ... I think treating tests that aren't run the same as SKIPs is certainly better than treating them as PASS. However, I'm wondering if we should have some other state to indicate 'skipped due to aborted run' or something. > > I thought about that. At first glance, it doesn't seem worth the extra complexity for such a rare occurrence ... > The complexity I refer to is not the code so much as having yet another thing that people working with the dashboard need to understand. I agree it's probably not worth the complexity in the code. Given that, the user thing becomes moot, but I'm not sure I agree with that part. I wouldn't argue too much one way or another, though.
Ojan Vafai
Comment 6 2012-03-30 16:47:17 PDT
(In reply to comment #1) > Curious, also in that screenshot is http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6/builds/14272 on the mac 10.6 bot. That ran to completion, but didn't run the fast/regions test and showed it as passing. Not sure what the issue is there, but it's definitely a bug (e.g. if we didn't run the test because it was skipped, we shouldn't show it as a PASS). Nevermind. This is the same issue. Instead of the buildbot harness killing it, NRWT is itself bailing early because of too many crashes.
Ojan Vafai
Comment 7 2012-03-30 16:52:25 PDT
As I think about this more, I think we *should* upload the results, but treat all the tests we didn't run as skipped.
Ojan Vafai
Comment 8 2012-03-31 13:14:23 PDT
Created attachment 134977 [details] Patch
Eric Seidel (no email)
Comment 9 2012-03-31 16:00:55 PDT
Comment on attachment 134977 [details] Patch Looks reasonable.
Ojan Vafai
Comment 10 2012-03-31 16:02:55 PDT
Committed r112805: <http://trac.webkit.org/changeset/112805>
Dirk Pranke
Comment 11 2012-04-10 16:50:32 PDT
This doesn't appear to have actually fixed things. Reopening to take a look.
Dirk Pranke
Comment 12 2012-04-10 19:24:04 PDT
Created attachment 136606 [details] Patch
Dirk Pranke
Comment 13 2012-04-10 19:38:23 PDT
Committed r113813: <http://trac.webkit.org/changeset/113813>
Note You need to log in before you can comment on or make changes to this bug.