| Summary: | implement first part of support for the new TestExpectations syntax | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||||
| Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth, dglazkov, eric, ojan, rniwa, tony | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 89161, 96588 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Dirk Pranke
2012-09-12 15:23:21 PDT
Created attachment 163745 [details]
clean up for review
This patch adds tests for tokenizing the new syntax and converting it into the old style so that the change is minimal to the rest of the code. This new parsing routine isn't actually used anywhere yet, so the new syntax isn't yet legal and hence there should be no functional changes. The next patch will allow for the new syntax to coexist alongside the old, and will write out all entries into the new format. Comment on attachment 163745 [details] clean up for review View in context: https://bugs.webkit.org/attachment.cgi?id=163745&action=review R- just for the Failure issue. > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:270 > + 'Failure': 'TEXT', Doesn't this need to be something like ['TEXT', 'IMAGE+TEXT', 'AUDIO']? So that any of those happening won't be an unexpected failure? > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:342 > + else: Maybe make this elif state == 'name_found' so you can log an error if the state is something else? > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:350 > + else: Ditto: elif state == 'expectations' and log an error otherwise? > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:378 > + if not expectations: > + if 'SKIP' not in modifiers and 'REBASELINE' not in modifiers and 'SLOW' not in modifiers: > + modifiers.append('SKIP') > + expectations = ['PASS'] Should this have a FIXME to be removed when we stop supporting Skipped files? > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:380 > + expectation_line.modifiers = bugs + modifiers Maybe have a FIXME to store bugs separately on the expectation_line? (In reply to comment #3) > (From update of attachment 163745 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163745&action=review > > R- just for the Failure issue. > > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:270 > > + 'Failure': 'TEXT', > > Doesn't this need to be something like ['TEXT', 'IMAGE+TEXT', 'AUDIO']? So that any of those happening won't be an unexpected failure? > The mapping is definitely an issue. I had been planning to post a separate patch that would remap failures onto the keywords in order to address this, but the idea of treating these things as "flaky" failures hadn't occurred to me, and that might be simpler. Another possibility is to replace TEXT, IMAGE+TEXT and AUDIO with FAIL before turning on the new syntax so that the mapping is 1:1. I haven't yet decided if that's better or worse than your suggestion. I think I'll code up both options and see. > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:342 > > + else: > > Maybe make this elif state == 'name_found' so you can log an error if the state is something else? > Ok. > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:350 > > + else: > > Ditto: elif state == 'expectations' and log an error otherwise? > Ok. > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:378 > > + if not expectations: > > + if 'SKIP' not in modifiers and 'REBASELINE' not in modifiers and 'SLOW' not in modifiers: > > + modifiers.append('SKIP') > > + expectations = ['PASS'] > > Should this have a FIXME to be removed when we stop supporting Skipped files? > I'm not sure I follow your thinking. You're suggesting that we should always require [ Skip ] or [ WontFix ] ? > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:380 > > + expectation_line.modifiers = bugs + modifiers > > Maybe have a FIXME to store bugs separately on the expectation_line? Ok. Comment on attachment 163745 [details] clean up for review View in context: https://bugs.webkit.org/attachment.cgi?id=163745&action=review >>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:270 >>> + 'Failure': 'TEXT', >> >> Doesn't this need to be something like ['TEXT', 'IMAGE+TEXT', 'AUDIO']? So that any of those happening won't be an unexpected failure? > > The mapping is definitely an issue. I had been planning to post a separate patch that would remap failures onto the keywords in order to address this, but the idea of treating these things as "flaky" failures hadn't occurred to me, and that might be simpler. > > Another possibility is to replace TEXT, IMAGE+TEXT and AUDIO with FAIL before turning on the new syntax so that the mapping is 1:1. I haven't yet decided if that's better or worse than your suggestion. > > I think I'll code up both options and see. Yup. Whichever one is easier is fine I think. >>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:378 >>> + expectations = ['PASS'] >> >> Should this have a FIXME to be removed when we stop supporting Skipped files? > > I'm not sure I follow your thinking. You're suggesting that we should always require [ Skip ] or [ WontFix ] ? Oh, right. I forgot that we were making the expectations part completely optional. nm. Although, maybe the FIXME should be to have the other code understand what it means for expectations to be an empty list instead of putting in the dummy PASS expectations. Not sure it matters. Comment on attachment 163745 [details] clean up for review View in context: https://bugs.webkit.org/attachment.cgi?id=163745&action=review > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:316 > + if (token.startswith('webkit.org/b/') or Given that we're implementing a finite automata here, I'd prefer us always checking the state first (effective switch statement on state). > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:318 > + token.startswith('bug(')): We should capitalize "Bug" as in "Bug(rniwa)". > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:324 > + if token.startswith('webkit.org/b/'): > + bugs.append(token.replace('webkit.org/b/', 'BUGWK')) > + elif token.startswith('crbug.com/'): It's not great that the list of URLs is hard-coded like this. It should be declared elsewhere so that people can easily add new URLs e.g. rdar, etc... > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:327 > + match = re.match('bug\((\w+)\)$', token) Ditto. (In reply to comment #6) > (From update of attachment 163745 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163745&action=review > > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:316 > > + if (token.startswith('webkit.org/b/') or > > Given that we're implementing a finite automata here, I'd prefer us always checking the state first (effective switch statement on state). > I suppose it depends on how you look at it. Right now the outer loop is an iteration over the list of tokens, not a proper FSA (which would require while state != done plus explicitly pulling the next token). That feels like it would be harder to follow to me. WDYT? > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:318 > > + token.startswith('bug(')): > > We should capitalize "Bug" as in "Bug(rniwa)". > Ok. I'm not a fan of this, but I'm not a fan of mixed case generally, and I can see the argument for consistency, so will change it :). > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:324 > > + if token.startswith('webkit.org/b/'): > > + bugs.append(token.replace('webkit.org/b/', 'BUGWK')) > > + elif token.startswith('crbug.com/'): > > It's not great that the list of URLs is hard-coded like this. It should be declared elsewhere so that people can easily add new URLs e.g. rdar, etc... > I think we've had this debate before. I don't particularly want people to be able to add new URLs (really, we should just be using webkit.org and even allowing crbug is lame in my book), I don't think we see much churn in the list, and it's not particularly harder to modify this code rather than reading in a list from a whole separate file somewhere, so I'm inclined to leave things as they are. (In reply to comment #7) > > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:324 > > > + if token.startswith('webkit.org/b/'): > > > + bugs.append(token.replace('webkit.org/b/', 'BUGWK')) > > > + elif token.startswith('crbug.com/'): > > > > It's not great that the list of URLs is hard-coded like this. It should be declared elsewhere so that people can easily add new URLs e.g. rdar, etc... > > > > I think we've had this debate before. I don't particularly want people to be able to add new URLs (really, we should just be using webkit.org and even allowing crbug is lame in my book), I don't think we see much churn in the list, and it's not particularly harder to modify this code rather than reading in a list from a whole separate file somewhere, so I'm inclined to leave things as they are. Okay. I've started a new webkit-dev thread about this. (In reply to comment #8) > (In reply to comment #7) > > > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:324 > > > > + if token.startswith('webkit.org/b/'): > > > > + bugs.append(token.replace('webkit.org/b/', 'BUGWK')) > > > > + elif token.startswith('crbug.com/'): > > > > > > It's not great that the list of URLs is hard-coded like this. It should be declared elsewhere so that people can easily add new URLs e.g. rdar, etc... > > > > > > > I think we've had this debate before. I don't particularly want people to be able to add new URLs (really, we should just be using webkit.org and even allowing crbug is lame in my book), I don't think we see much churn in the list, and it's not particularly harder to modify this code rather than reading in a list from a whole separate file somewhere, so I'm inclined to leave things as they are. > > Okay. I've started a new webkit-dev thread about this. Great. I'm happy to implement whatever the consensus is. (In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:324 > > > > > + if token.startswith('webkit.org/b/'): > > > > > + bugs.append(token.replace('webkit.org/b/', 'BUGWK')) > > > > > + elif token.startswith('crbug.com/'): > > > > > > > > It's not great that the list of URLs is hard-coded like this. It should be declared elsewhere so that people can easily add new URLs e.g. rdar, etc... > > > > > > > > > > I think we've had this debate before. I don't particularly want people to be able to add new URLs (really, we should just be using webkit.org and even allowing crbug is lame in my book), I don't think we see much churn in the list, and it's not particularly harder to modify this code rather than reading in a list from a whole separate file somewhere, so I'm inclined to leave things as they are. > > > > Okay. I've started a new webkit-dev thread about this. > > Great. I'm happy to implement whatever the consensus is. This is easy to change later. So, no reason to block this review on the discussion resolving IMO. (In reply to comment #10) > > This is easy to change later. So, no reason to block this review on the discussion resolving IMO. Agreed. Created attachment 164782 [details]
merge to head, address review comments, patch for landing
Comment on attachment 164782 [details] merge to head, address review comments, patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=164782&action=review > Tools/ChangeLog:34 > + - We use webkit.org/b/12345, crbug.com/12345, and bug(dpranke) Nit: bug -> Bug. > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:274 > + 'Failure': 'TEXT', !? We've got rid of TEXT, no? Comment on attachment 164782 [details] merge to head, address review comments, patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=164782&action=review > Tools/ChangeLog:26 > + - TEXT, AUDIO, IMAGE+TEXT -> Failure - FAIL -> Failure > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:274 > + 'Failure': 'TEXT', s/TEXT/FAIL ? (In reply to comment #13) > (From update of attachment 164782 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164782&action=review > > > Tools/ChangeLog:34 > > + - We use webkit.org/b/12345, crbug.com/12345, and bug(dpranke) > > Nit: bug -> Bug. Fixed. > > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:274 > > + 'Failure': 'TEXT', > > !? We've got rid of TEXT, no? Yup, bad merge. Fixed. (In reply to comment #14) > (From update of attachment 164782 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164782&action=review > > > Tools/ChangeLog:26 > > + - TEXT, AUDIO, IMAGE+TEXT -> Failure > > - FAIL -> Failure > Fixed. > > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:274 > > + 'Failure': 'TEXT', > > s/TEXT/FAIL ? :). Created attachment 164787 [details]
more review feedback
Committed r129051: <http://trac.webkit.org/changeset/129051> |