Bug 183796 - Lint web-platform-tests changes with the wpt linter before exporting
Summary: Lint web-platform-tests changes with the wpt linter before exporting
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-20 13:36 PDT by Brendan McLoughlin
Modified: 2018-03-23 14:20 PDT (History)
7 users (show)

See Also:


Attachments
Patch (95.89 KB, patch)
2018-03-20 13:50 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (96.67 KB, patch)
2018-03-20 14:06 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (110.02 KB, patch)
2018-03-21 06:52 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (124.02 KB, patch)
2018-03-21 07:59 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2018-03-22 11:14 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (7.86 KB, patch)
2018-03-22 11:35 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (7.86 KB, patch)
2018-03-22 11:50 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (8.46 KB, patch)
2018-03-23 11:57 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brendan McLoughlin 2018-03-20 13:36:46 PDT
Lint web-platform-tests changes with the wpt linter before exporting
Comment 1 Brendan McLoughlin 2018-03-20 13:50:41 PDT
Created attachment 336148 [details]
Patch
Comment 2 Brendan McLoughlin 2018-03-20 14:06:53 PDT
Created attachment 336149 [details]
Patch
Comment 3 Brendan McLoughlin 2018-03-21 06:52:54 PDT
Created attachment 336189 [details]
Patch
Comment 4 Brendan McLoughlin 2018-03-21 07:59:25 PDT
Created attachment 336193 [details]
Patch
Comment 5 youenn fablet 2018-03-21 09:09:18 PDT
Comment on attachment 336193 [details]
Patch

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

> Tools/Scripts/webkitpy/w3c/test_exporter.py:46
> +sys.path.append(WEBKIT_WPT_DIR)

I wonder whether we have another way of doing this. Jonathan, Dean?
Maybe this explains why webkitpy bot is failing here.

Brendan, you can try to do what the bot is doing to reproduce the issue:
/Volumes/.. /Tools/Scripts/webkit-patch build-and-test --no-clean --no-update --test --non-interactive --build-style=release --group=webkitpy --port=mac

> Tools/Scripts/webkitpy/w3c/test_exporter.py:231
> +        return filter(lambda x: x.startswith(WEBKIT_WPT_DIR), changed_files)

Is it working for all options passed to the exporter, like if a specific commit is specified?
I also wonder whether we should do the lint once we applied the patch to the wpt repo.
That would ensure that we use the latest linter from WPT and not the one from WebKit repo.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:235
> +        repo_root = os.path.abspath(os.path.join(here, os.pardir, os.pardir, os.pardir, os.pardir, 'LayoutTests', 'imported', 'w3c', 'web-platform-tests'))

We usually try to use self._filesystem and not os.path directly.
At some point, we might also want to have check-webkit-style do some lint, so maybe there should be a dedicated class for a WPT lint wrapper.
Comment 6 Jonathan Bedard 2018-03-21 09:44:14 PDT
Comment on attachment 336193 [details]
Patch

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

>> Tools/Scripts/webkitpy/w3c/test_exporter.py:46
>> +sys.path.append(WEBKIT_WPT_DIR)
> 
> I wonder whether we have another way of doing this. Jonathan, Dean?
> Maybe this explains why webkitpy bot is failing here.
> 
> Brendan, you can try to do what the bot is doing to reproduce the issue:
> /Volumes/.. /Tools/Scripts/webkit-patch build-and-test --no-clean --no-update --test --non-interactive --build-style=release --group=webkitpy --port=mac

Unless the containing script added this directory the python path, sys.path.append is probably the best way to do this. There are some other nasty import magic stuff you can do, but I think that makes things more difficult.
I believe this comment should read 'Add web-platform-tests to the python path so we can import the WPT lint module'

You also want to do something like this to handle people running from weird directories:

WEBKIT_WPT_DIR = os.join(os.path.dirname(os.path.abspath(__file__)), '..', '..', '..', 'LayoutTests', 'imported', 'w3c', 'web-platform-tests'')
...
sys.path.append(WEBKIT_WPT_DIR)

I would also prefer that we append 'LayoutTests/imported/w3c' and future imports would be 'from <something WPT unique>.tools.lint.lint import ....' The trouble with this is that 'web-platform-tests' is an invalid module name in Python.
Comment 7 Brendan McLoughlin 2018-03-22 11:14:39 PDT
Created attachment 336290 [details]
Patch
Comment 8 Build Bot 2018-03-22 11:16:27 PDT
Attachment 336290 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:142:  [TestExporterTest.test_export] Instance of 'WPTLinter' has no 'calls' member (but some types could not be inferred)  [pylint/E1103] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Brendan McLoughlin 2018-03-22 11:35:07 PDT
Created attachment 336292 [details]
Patch
Comment 10 Build Bot 2018-03-22 11:37:47 PDT
Attachment 336292 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/w3c/test_exporter.py:37:  No name 'wpt_lint' in module 'webkitpy.w3c'  [pylint/E0611] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Brendan McLoughlin 2018-03-22 11:50:06 PDT
Created attachment 336294 [details]
Patch
Comment 12 youenn fablet 2018-03-22 14:24:08 PDT
Comment on attachment 336294 [details]
Patch

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

> Tools/Scripts/webkitpy/w3c/wpt_linter.py:31
> +    calls = []

Probably not needed.

> Tools/Scripts/webkitpy/w3c/wpt_linter.py:37
> +        cmd = [self.repository_directory + '/wpt', 'lint']

We usually use webkitpy filesystem.join to concat paths appropriately.
Maybe you could pass the filesystem to the constructor and build the path there.
Comment 13 Brendan McLoughlin 2018-03-23 11:31:30 PDT
Comment on attachment 336294 [details]
Patch

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

>> Tools/Scripts/webkitpy/w3c/wpt_linter.py:31
>> +    calls = []
> 
> Probably not needed.

When I remove this line I get the following linter error:

ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:142:  [TestExporterTest.test_export] Instance of 'WPTLinter' has no 'calls' member (but some types could not be inferred)  [pylint/E1103] [5]

Here the linter wrong, since the reference to `calls` on line 142 in test_exporter_unittest.py is actually an instance of MockWPTLinter but I wasn't able to figure out a way to signal that to the linter and I'm not sure why the other Mock objects used in test_exporter_unittest.py also don't trigger the same volition.

>> Tools/Scripts/webkitpy/w3c/wpt_linter.py:37
>> +        cmd = [self.repository_directory + '/wpt', 'lint']
> 
> We usually use webkitpy filesystem.join to concat paths appropriately.
> Maybe you could pass the filesystem to the constructor and build the path there.

Will do.
Comment 14 youenn fablet 2018-03-23 11:46:23 PDT
> >> Tools/Scripts/webkitpy/w3c/wpt_linter.py:31
> >> +    calls = []
> > 
> > Probably not needed.
> 
> When I remove this line I get the following linter error:
> 
> ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:142: 
> [TestExporterTest.test_export] Instance of 'WPTLinter' has no 'calls' member
> (but some types could not be inferred)  [pylint/E1103] [5]
> 
> Here the linter wrong, since the reference to `calls` on line 142 in
> test_exporter_unittest.py is actually an instance of MockWPTLinter but I
> wasn't able to figure out a way to signal that to the linter and I'm not
> sure why the other Mock objects used in test_exporter_unittest.py also don't
> trigger the same volition.


It might be that the linter has been improved since the time we committed the changes?
Maybe you could keep the Mock linter in a variable and check its calls member directly instead of accessing through the exporter.
Comment 15 Brendan McLoughlin 2018-03-23 11:54:48 PDT
(In reply to youenn fablet from comment #14)
> 
> It might be that the linter has been improved since the time we committed
> the changes?
> Maybe you could keep the Mock linter in a variable and check its calls
> member directly instead of accessing through the exporter.

I'll give that a try.
Comment 16 Brendan McLoughlin 2018-03-23 11:57:20 PDT
Created attachment 336399 [details]
Patch
Comment 17 youenn fablet 2018-03-23 13:58:54 PDT
Comment on attachment 336399 [details]
Patch

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

> Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:117
> +            mock_linter = self

We should probably empty self.calls otherwise it will grow for each sub test.
Comment 18 youenn fablet 2018-03-23 14:08:12 PDT
Comment on attachment 336399 [details]
Patch

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

>> Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:117
>> +            mock_linter = self
> 
> We should probably empty self.calls otherwise it will grow for each sub test.

Was wrong about that, no need for emptying it.
Comment 19 WebKit Commit Bot 2018-03-23 14:19:42 PDT
Comment on attachment 336399 [details]
Patch

Clearing flags on attachment: 336399

Committed r229921: <https://trac.webkit.org/changeset/229921>
Comment 20 WebKit Commit Bot 2018-03-23 14:19:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2018-03-23 14:20:19 PDT
<rdar://problem/38807006>