Bug 196016 - webkit-patch upload doesn't run 'check-style-local' so doesn't find TestExpectations errors
Summary: webkit-patch upload doesn't run 'check-style-local' so doesn't find TestExpec...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-20 10:44 PDT by Simon Fraser (smfr)
Modified: 2021-08-11 07:06 PDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-03-20 10:44:43 PDT
I just uploaded a patch where a file added to TestExpectations didn't exist (https://bugs.webkit.org/attachment.cgi?id=365355&action=prettypatch), but webkit-patch didn't warn me.

Style checker runs 'check-style-local' but webkit-patch doesn't do that?
Comment 1 Simon Fraser (smfr) 2019-03-20 10:45:59 PDT
lint-test-expectations seems really broken; we should remove it.
Comment 2 Jonathan Bedard 2019-03-20 10:47:47 PDT
I'm a bit confused.

You're saying that EWS caught it but webkit-patch didn't?
Comment 3 Jonathan Bedard 2019-03-20 10:57:07 PDT
Looking a bit at the code and doing some local testing:

lint-test-expectations is weird because we have a whole bunch of defunct buildbot stuff sitting around in our code which defines ports automation is using. This stuff gets out of date super fast, and we shouldn't rely on it (lint-test-expectations does rely on it)

What's more, lint-test-expectations does really weird things with port object when you try and define it (via --ios-simulator, for example)

With all that being said, the core logic of lint-test-expectations is not the problem. Its port management is. I think looking at Simon's patch, 'lint-test-expectations' with no arguments might have actually flagged what he wanted it to.
Comment 4 Simon Fraser (smfr) 2019-03-20 11:48:37 PDT
(In reply to Jonathan Bedard from comment #2)
> I'm a bit confused.
> 
> You're saying that EWS caught it but webkit-patch didn't?

Yes
Comment 5 Simon Fraser (smfr) 2019-03-20 11:49:01 PDT
'lint-test-expectations' with no arguments told me about Windows expectations.
Comment 6 Jonathan Bedard 2019-03-20 12:50:42 PDT
(In reply to Simon Fraser (smfr) from comment #5)
> 'lint-test-expectations' with no arguments told me about Windows
> expectations.

That's what it should do (along with the iOS ones). lint-test-expectations basically attempts to lint expectations for multiple ports.
Comment 7 Alexey Proskuryakov 2019-03-22 10:38:20 PDT
lint-test-expectations doesn't produce the same result as "run-webkit-tests --lint". I have actually never heard of the former, why do we have two ways to lint?

But the difference between two tools seems at best tangentially related to the issue at hand. webkit-patch is supposed to check style by default AFAIK, so it seems surprising that it finds fewer issues than the style EWS.

Funny that we also have bug 102009 which claims that webkit-patch ALWAYS complains about TestExpectations.