Bug 189293

Summary: Log when leak detection changes the test result
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, ews-watchlist, glenn, jbedard, jonlee, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 186214    
Attachments:
Description Flags
Patch
none
Patch jonlee: review+, commit-queue: commit-queue-

Description Simon Fraser (smfr) 2018-09-04 18:19:40 PDT
Log when leak detection changes the test result
Comment 1 Simon Fraser (smfr) 2018-09-04 18:21:25 PDT
Created attachment 348883 [details]
Patch
Comment 2 Jonathan Bedard 2018-09-05 14:00:08 PDT
Comment on attachment 348883 [details]
Patch

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

> Tools/ChangeLog:10
> +        When you have an "Leak" expectation in TestExpectations, it's confusing to see a test unexpectedly pass,
> +        but then show up as an expected fail at the end (because leak detection happens at the end of a shard).
> +        So log when leak detection changes a test result.

Does this change actually address the fact that a test will show up as an unexpected pass until the leak detection happens? Or does it just log when the result changes after leak detection occurs?

It looks like this does the second. I'd like to eventually do the first, but this seems like a decent mitigation.
Comment 3 Simon Fraser (smfr) 2018-09-05 14:47:02 PDT
(In reply to Jonathan Bedard from comment #2)
> Comment on attachment 348883 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348883&action=review
> 
> > Tools/ChangeLog:10
> > +        When you have an "Leak" expectation in TestExpectations, it's confusing to see a test unexpectedly pass,
> > +        but then show up as an expected fail at the end (because leak detection happens at the end of a shard).
> > +        So log when leak detection changes a test result.
> 
> Does this change actually address the fact that a test will show up as an
> unexpected pass until the leak detection happens? Or does it just log when
> the result changes after leak detection occurs?
>
> It looks like this does the second. I'd like to eventually do the first, but
> this seems like a decent mitigation.

The latter. Leak detect is async, so the only other option would be to log something like: "fast/css/user-drag-none.html passed, but might leak", and then something when the leak is detected.
Comment 4 Jonathan Bedard 2018-09-05 15:08:18 PDT
(In reply to Simon Fraser (smfr) from comment #3)
> (In reply to Jonathan Bedard from comment #2)
> > Comment on attachment 348883 [details]
> > ...
> 
> The latter. Leak detect is async, so the only other option would be to log
> something like: "fast/css/user-drag-none.html passed, but might leak", and
> then something when the leak is detected.

So I like the other option for tests that are expected to leak.

There is a third (but much more painful) option: we could defer printing ALL results until leak checking is finished. But that's a pretty non-trivial and controversial change.
Comment 5 Simon Fraser (smfr) 2018-09-06 15:36:44 PDT
Created attachment 349084 [details]
Patch
Comment 6 Simon Fraser (smfr) 2018-09-06 15:38:17 PDT
Output: 

Unexpected leak:
[1/1] fast/css/user-drag-none.html passed
  fast/css/user-drag-none.html -> changed by leak detection from a pass (expected) to a leak (unexpected)

Expected leak:
[1/1] fast/css/user-drag-none.html passed (leak detection is pending)
  fast/css/user-drag-none.html -> changed by leak detection from a pass (unexpected) to a leak (expected)
Comment 7 WebKit Commit Bot 2018-09-06 17:02:24 PDT
Comment on attachment 349084 [details]
Patch

Rejecting attachment 349084 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 349084, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=349084&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=189293&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 349084 from bug 189293.
Fetching: https://bugs.webkit.org/attachment.cgi?id=349084
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Jon Lee']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 4 diffs from patch file(s).
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py
Hunk #1 FAILED at 200.
1 out of 1 hunk FAILED -- saving rejects to file Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py.rej
patching file Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py
Hunk #1 succeeded at 815 (offset 14 lines).
patching file Tools/Scripts/webkitpy/layout_tests/views/printing.py

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Jon Lee']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/9120256
Comment 8 Simon Fraser (smfr) 2018-09-06 17:25:19 PDT
https://trac.webkit.org/r235771
Comment 9 Radar WebKit Bug Importer 2018-09-06 17:28:04 PDT
<rdar://problem/44204528>