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.
Created attachment 400738 [details] Patch
Created attachment 400803 [details] Patch
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.
(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)
(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) >
> 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.
(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.
Created attachment 400849 [details] Patch rework patch and add more unit tests
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.
ping? r?
Committed r262565: <https://trac.webkit.org/changeset/262565> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400849 [details].
<rdar://problem/63992453>