Bug 96569 – implement first part of support for the new TestExpectations syntax

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

implement first part of support for the new TestExpectations syntax

[Dirk Pranke]

Created attachment 163745 [details]
clean up for review

[Dirk Pranke]

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.

[Ojan Vafai]

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?

[Dirk Pranke]

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

[Ojan Vafai]

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.

[Ryosuke Niwa]

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.

[Dirk Pranke]

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

[Ryosuke Niwa]

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

[Dirk Pranke]

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

[Ojan Vafai]

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

[Ryosuke Niwa]

(In reply to comment #10)
>
> This is easy to change later. So, no reason to block this review on the discussion resolving IMO.

Agreed.

[Dirk Pranke]

Created attachment 164782 [details]
merge to head, address review comments, patch for landing

[Ryosuke Niwa]

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?

[Ojan Vafai]

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 ?

[Dirk Pranke]

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

:).

[Dirk Pranke]

Created attachment 164787 [details]
more review feedback

[Dirk Pranke]

Committed r129051: <http://trac.webkit.org/changeset/129051>