Bug 183796 – Lint web-platform-tests changes with the wpt linter before exporting

• Component: New Bugs
• Status: RESOLVED FIXED

[Brendan McLoughlin]

Lint web-platform-tests changes with the wpt linter before exporting

[Brendan McLoughlin]

Created attachment 336148 [details]
Patch

[Brendan McLoughlin]

Created attachment 336149 [details]
Patch

[Brendan McLoughlin]

Created attachment 336189 [details]
Patch

[Brendan McLoughlin]

Created attachment 336193 [details]
Patch

[youenn fablet]

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.

[Jonathan Bedard]

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.

[Brendan McLoughlin]

Created attachment 336290 [details]
Patch

[EWS Watchlist]

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.

[Brendan McLoughlin]

Created attachment 336292 [details]
Patch

[EWS Watchlist]

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.

[Brendan McLoughlin]

Created attachment 336294 [details]
Patch

[youenn fablet]

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.

[Brendan McLoughlin]

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.

[youenn fablet]

> >> 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.

[Brendan McLoughlin]

(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.

[Brendan McLoughlin]

Created attachment 336399 [details]
Patch

[youenn fablet]

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.

[youenn fablet]

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.

[WebKit Commit Bot]

Comment on attachment 336399 [details]
Patch

Clearing flags on attachment: 336399

Committed r229921: <https://trac.webkit.org/changeset/229921>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38807006>