Bug 189088

Summary: Make it possible to mark tests as leaks in TestExpectations
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, ews-watchlist, glenn, jbedard, jonlee, simon.fraser, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 186214    
Attachments:
Description Flags
Patch
none
Patch
none
Patch jonlee: review+

Description Simon Fraser (smfr) 2018-08-29 10:25:48 PDT
Make it possible to mark tests as leaks in TestExpectations
Comment 1 Simon Fraser (smfr) 2018-08-29 10:28:41 PDT
Created attachment 348411 [details]
Patch
Comment 2 Simon Fraser (smfr) 2018-08-29 10:29:34 PDT
Needs the patch from bug 189067.
Comment 3 Jonathan Bedard 2018-08-29 12:42:51 PDT
Is the intention to have this enabled by default or to have a queue explicitly testing it?
Comment 4 Simon Fraser (smfr) 2018-08-29 12:59:36 PDT
Enabled by default on some platforms.
Comment 5 Simon Fraser (smfr) 2018-08-29 13:02:29 PDT
Created attachment 348424 [details]
Patch
Comment 6 Alexey Proskuryakov 2018-08-29 13:55:53 PDT
Comment on attachment 348424 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:75
> +Bug(test) failures/expected/leaky-reftest.html [ ImageOnlyFailure Leak ]

I still think that "Leak" is a confusing name for this - I definitely wouldn't think that it has any relationship to document leaks.
Comment 7 Simon Fraser (smfr) 2018-08-29 14:51:50 PDT
I went with "leak" because "abandoned document" is too long, we may want to track other types of leaked objects (e.g. Frames) later, and it's consistent with the "world leak" term that WebKit has used historically.
Comment 8 Simon Fraser (smfr) 2018-08-29 15:11:57 PDT
Created attachment 348435 [details]
Patch
Comment 9 Simon Fraser (smfr) 2018-08-30 09:32:14 PDT
Review ping?
Comment 10 Jon Lee 2018-08-30 09:53:41 PDT
Comment on attachment 348435 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:-248
> -        'Leak': 'LEAK',

There are enough here that maybe the list should just be alphabetized...?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:868
>              expected_results.add(PASS)

Not part of your patch, but it seems like this is wrong.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:136
>          self.assertEqual(TestExpectations.remove_pixel_failures(set([PASS, IMAGE, CRASH])), set([PASS, CRASH]))

Maybe add some test cases with LEAK here?
Comment 11 Simon Fraser (smfr) 2018-08-31 13:03:32 PDT
https://trac.webkit.org/r235564
Comment 12 Radar WebKit Bug Importer 2018-08-31 13:04:18 PDT
<rdar://problem/43952197>