Bug 186261

Summary: import-w3c-tests should rely on <meta name="flags"> to detect CSS manual tests
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: Tools / TestsAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, glenn, lforschler, Ms2ger, rego, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/issues/5381
Attachments:
Description Flags
Patch youennf: review+

Description Frédéric Wang (:fredw) 2018-06-03 23:24:53 PDT 
CSS tests have additional flags, which can be used to indicate that the tests are manual:

https://web-platform-tests.org/writing-tests/css-metadata.html#requirement-flags

From WPT's tools/manifest/sourcefile.py:

def content_is_css_manual(self):
        """Boolean indicating whether the file content represents a
        CSS WG-style manual test"""
        if self.root is None:
            return None
        # return True if the intersection between the two sets is non-empty
return bool(self.css_flags & {"animated", "font", "history", "interact", "paged", "speech", "userstyle"})

Currently, import-w3c-tests just treat them as "support" files, instead of ignoring them.
Comment 1 Frédéric Wang (:fredw) 2018-06-04 00:12:01 PDT 
Created attachment 341895 [details]
Patch
Comment 2 Ms2ger (he/him; ⌚ UTC+1/+2) 2018-06-04 00:44:51 PDT 
LGTM
Comment 3 youenn fablet 2018-06-04 17:56:21 PDT 
Comment on attachment 341895 [details]
Patch

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

> Tools/ChangeLog:14
> +        <meta name="flags"> are manual are non-manual according to CSS WG rules.

s/are/or/

> Tools/Scripts/webkitpy/w3c/test_parser.py:119
> +        elif self.is_wpt_manualtest() or self.is_css_manualtest():

We should probably merge is_wpt_manualtest and is_css_manualtest

> Tools/Scripts/webkitpy/w3c/test_parser_unittest.py:183
> +            self.assertEqual(test_info, None, 'test_info is not None')

Maybe we should add asis which is a valid css flag but not classified as manual.

> Tools/Scripts/webkitpy/w3c/test_parser_unittest.py:194
> +            self.assertTrue(test_info['manualtest'], 'test with CSS flag %s is not manual' % flag)

I never know whether the assertion message tells what is expected or what is unexpected.
I would rewrite it to 'test with CSS flag %s should be manual'
Comment 4 Frédéric Wang (:fredw) 2018-06-05 02:13:31 PDT 
Comment on attachment 341895 [details]
Patch

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

>> Tools/ChangeLog:14
>> +        <meta name="flags"> are manual are non-manual according to CSS WG rules.
> 
> s/are/or/

Done

>> Tools/Scripts/webkitpy/w3c/test_parser.py:119
>> +        elif self.is_wpt_manualtest() or self.is_css_manualtest():
> 
> We should probably merge is_wpt_manualtest and is_css_manualtest

Done

>> Tools/Scripts/webkitpy/w3c/test_parser_unittest.py:183
>> +            self.assertEqual(test_info, None, 'test_info is not None')
> 
> Maybe we should add asis which is a valid css flag but not classified as manual.

Done.

>> Tools/Scripts/webkitpy/w3c/test_parser_unittest.py:194
>> +            self.assertTrue(test_info['manualtest'], 'test with CSS flag %s is not manual' % flag)
> 
> I never know whether the assertion message tells what is expected or what is unexpected.
> I would rewrite it to 'test with CSS flag %s should be manual'

Yeah, I was also confused by existing tests. Done here and as well above "test_info should be None"
Comment 5 Frédéric Wang (:fredw) 2018-06-05 02:18:16 PDT 
Committed r232506: <https://trac.webkit.org/changeset/232506>
Comment 6 Radar WebKit Bug Importer 2018-06-05 02:19:27 PDT 
<rdar://problem/40800382>