Bug 212597 – Improve watchlist logic for comments on patches touching imported WPT tests.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Alberto Lopez Perez]

On r262295 I added a watchlist comment for patches touching the imported WPT tests.

However this is commenting on patches that are trying to import WPT tests.

I guess we can improve the logic to avoid commenting on patches that touch "w3c-import.log" files, since that indicates the changes on the WPT tests come from an import.

[Carlos Alberto Lopez Perez]

Created attachment 400738 [details]
Patch

[Carlos Alberto Lopez Perez]

Created attachment 400803 [details]
Patch

[youenn fablet]

Comment on attachment 400803 [details]
Patch

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

> Tools/Scripts/webkitpy/common/config/watchlist:384
> +            "filename": r"LayoutTests/imported/w3c/web-platform-tests/.*w3c-import.log$",

This rule does not guarantee that we will identify all WPT import patches but this might be good enough

> Tools/Scripts/webkitpy/common/watchlist/watchlistrule.py:44
> +        for test_definition in self.rule_to_match.split('|'):

Mixing & and | may make things complicated, for instance which one takes precedence.
How about replacing support of | by copy/pasting lines. | seems only used for one rule at the moment.

[Carlos Alberto Lopez Perez]

(In reply to youenn fablet from comment #3)
> Comment on attachment 400803 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400803&action=review
> 
> > Tools/Scripts/webkitpy/common/config/watchlist:384
> > +            "filename": r"LayoutTests/imported/w3c/web-platform-tests/.*w3c-import.log$",
> 
> This rule does not guarantee that we will identify all WPT import patches
> but this might be good enough
> 
> > Tools/Scripts/webkitpy/common/watchlist/watchlistrule.py:44
> > +        for test_definition in self.rule_to_match.split('|'):
> 
> Mixing & and | may make things complicated, for instance which one takes
> precedence.

They take the same precedence. It simply evaluates from left to right. Parenthesis are not supported

An expression like "a|b&c|d&e&!f|!g" should evaluate to -> (a) || ( b && c) || ( d && e && !f) || (!g)

I added some unit tests to double-check this is the case.

> How about replacing support of | by copy/pasting lines. | seems only used
> for one rule at the moment.

I don't see the befit of doing that, it would make the watchlist bigger and remove support for an useful feature (being able to OR the rules)

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #4)
> (In reply to youenn fablet from comment #3)
> > Comment on attachment 400803 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=400803&action=review
> > 
> > > Tools/Scripts/webkitpy/common/config/watchlist:384
> > > +            "filename": r"LayoutTests/imported/w3c/web-platform-tests/.*w3c-import.log$",
> > 
> > This rule does not guarantee that we will identify all WPT import patches
> > but this might be good enough
> > 
> > > Tools/Scripts/webkitpy/common/watchlist/watchlistrule.py:44
> > > +        for test_definition in self.rule_to_match.split('|'):
> > 
> > Mixing & and | may make things complicated, for instance which one takes
> > precedence.
> 
> They take the same precedence. It simply evaluates from left to right.
> Parenthesis are not supported



Mmm.. actually it doesn't.. after re-reading what I wrote, I see that it first evaluates all the ORs and then the expressions inside the ORs (which can have ANDs)

> 
> An expression like "a|b&c|d&e&!f|!g" should evaluate to -> (a) || ( b && c)
> || ( d && e && !f) || (!g)
>

[youenn fablet]

> An expression like "a|b&c|d&e&!f|!g" should evaluate to -> (a) || ( b && c)
> || ( d && e && !f) || (!g)

OK, that reads fine I guess.

With the patch, we are parsing twice the expression.
Once doing split('|') and once doing re.split('&|\|', complex_definition.replace('!', '')).
This probably does not give the same results.

Can we make the patch so that self.definitions_to_match still contains complex_definition.split('|'), and verify all users of definitions_to_match are fine with the & and ! that might be inside the items.

[Carlos Alberto Lopez Perez]

(In reply to youenn fablet from comment #6)
> > An expression like "a|b&c|d&e&!f|!g" should evaluate to -> (a) || ( b && c)
> > || ( d && e && !f) || (!g)
> 
> OK, that reads fine I guess.
> 
> With the patch, we are parsing twice the expression.
> Once doing split('|') and once doing re.split('&|\|',
> complex_definition.replace('!', '')).
> This probably does not give the same results.
> 

The code tries to ensure that all defined rules are used (and that you don't use any undefined rule). So it wants a list of definitions to check if all of them are used.

That's why that replace.. to just extract a list of rules (without the logic operators)


> Can we make the patch so that self.definitions_to_match still contains
> complex_definition.split('|'), and verify all users of definitions_to_match
> are fine with the & and ! that might be inside the items.

Sure.. I will rework the code so that we only store in the class the expression as is it, and the consumers of it (the code to extract the list of definitions and the one to parse the logic) each one take care of correctly parsing it.

[Carlos Alberto Lopez Perez]

Created attachment 400849 [details]
Patch

rework patch and add more unit tests

[Carlos Alberto Lopez Perez]

BTW, the webkitpy failure its unrelated to this patch. Its a pre-existent issue that happened today meanwhile the bots were offline due to the S3 issue.

[Carlos Alberto Lopez Perez]

ping? r?

[EWS]

Committed r262565: <https://trac.webkit.org/changeset/262565>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400849 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63992453>