Bug 83347 – webkit-patch: add a print-expectations command

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

[Dirk Pranke]

webkit-patch: add a print-expectations command

[Dirk Pranke]

Created attachment 135976 [details]
Patch

[Dirk Pranke]

if this looks like reasonable functionality, let me know and I'll write some unit tests to go along with it.

[Dirk Pranke]

it's basically a view onto the various TestExpectationModels.

[Dirk Pranke]

Note that this will replace the skipped-files command which will let me finally fix bug 47528

[Adam Barth]

Comment on attachment 135976 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:751
> +        return (keyword.upper() in self.get_expectations_string(test) or
> +                keyword.lower() in self.get_modifiers(test))

Why is one upper and the other lower?

> Tools/Scripts/webkitpy/tool/commands/queries.py:410
> +    TYPES = set(x.upper() for x in (TestExpectations.EXPECTATIONS.keys() + TestExpectations.MODIFIERS.keys())).difference(set(['NONE']))

Can we use a more descriptive variable name than "x" ?  Perhaps "key" or "type" ?

> Tools/Scripts/webkitpy/tool/commands/queries.py:420
> +             make_option('-i', '--include-keyword', action='append', default=[],

bad indent

> Tools/Scripts/webkitpy/tool/commands/queries.py:434
> +    def get_expectations(self, tool, options, port_name, tests):

"get" is a pretty weak verb.  Also, I don't think you need to pass "tool" as a parameter.  It should be on self.  (I know we pass the tool around in a bunch of places---that's from code that predates tool being on self.)

> Tools/Scripts/webkitpy/tool/commands/queries.py:452
> +        default_port = tool.port_factory.get(options=options)

The port_factory understands our options?  Does that means some of these make_option calls are duplicated elsewhere?  Should move centralize them somewhere?

> Tools/Scripts/webkitpy/tool/commands/queries.py:490
> +    def print_expectation(self, options, test_name, expectations, port_name, port_names):
> +        modifiers_str = ' '.join(expectations.get_modifiers(test_name)).upper()
> +        expectations_str = expectations.get_expectations_string(test_name)
> +        if options.csv:
> +            print "%s,%s,%s,%s" % (port_name, test_name, modifiers_str, expectations_str)
> +        elif options.full:
> +            print "%s : %s = %s" % (modifiers_str, test_name, expectations_str)
> +        else:
> +            res = test_name
> +            if self.should_print_expectation(options):
> +                res += ' = ' + expectations.get_expectations_string(test_name)
> +            print res

This code seems too detail oriented for a command.  Maybe TestExpectations should know how to serialize itself back to a string and we could just print the string?

[Adam Barth]

I'm not 100% sure what I'd use this command for, but it seems like a reasonable thing to do if you've got some use in mind.

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 135976 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135976&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:751
> > +        return (keyword.upper() in self.get_expectations_string(test) or
> > +                keyword.lower() in self.get_modifiers(test))
> 
> Why is one upper and the other lower?
> 

Unfortunately, get_modifiers() returns things in lower case and get_expectations_string() returns things in upper case. My intent is that has_keyword() should be case insensitive. Initially I had implemented this outside of TestExpectations, but I decided I didn't want callers to have to care or be aware of the case of these things at all.

> > Tools/Scripts/webkitpy/tool/commands/queries.py:410
> > +    TYPES = set(x.upper() for x in (TestExpectations.EXPECTATIONS.keys() + TestExpectations.MODIFIERS.keys())).difference(set(['NONE']))
> 
> Can we use a more descriptive variable name than "x" ?  Perhaps "key" or "type" ?
>

Sure.
 
> > Tools/Scripts/webkitpy/tool/commands/queries.py:420
> > +             make_option('-i', '--include-keyword', action='append', default=[],
> 
> bad indent
> 

Will fix (not sure how I missed this).

> > Tools/Scripts/webkitpy/tool/commands/queries.py:434
> > +    def get_expectations(self, tool, options, port_name, tests):
> 
> "get" is a pretty weak verb.  Also, I don't think you need to pass "tool" as a parameter.  It should be on self.  (I know we pass the tool around in a bunch of places---that's from code that predates tool being on self.)
>

would expectations() be better? lookup_expectations()? I'm open to other suggestions.

There is a self._tool, but I wasn't sure if that was intended to be accessed in subclasses. Good to know.
 
> > Tools/Scripts/webkitpy/tool/commands/queries.py:452
> > +        default_port = tool.port_factory.get(options=options)
> 
> The port_factory understands our options?  Does that means some of these make_option calls are duplicated elsewhere?  Should move centralize them somewhere?
>

Yes, and probably. I am intentionally supporting a bunch of common options like --platform and --debug. We should probably centralize them on PortFactory (e.g., PortFactory.options()). I'll do that in a separate change.
 
> > Tools/Scripts/webkitpy/tool/commands/queries.py:490
> > +    def print_expectation(self, options, test_name, expectations, port_name, port_names):
> > +        modifiers_str = ' '.join(expectations.get_modifiers(test_name)).upper()
> > +        expectations_str = expectations.get_expectations_string(test_name)
> > +        if options.csv:
> > +            print "%s,%s,%s,%s" % (port_name, test_name, modifiers_str, expectations_str)
> > +        elif options.full:
> > +            print "%s : %s = %s" % (modifiers_str, test_name, expectations_str)
> > +        else:
> > +            res = test_name
> > +            if self.should_print_expectation(options):
> > +                res += ' = ' + expectations.get_expectations_string(test_name)
> > +            print res
> 
> This code seems too detail oriented for a command.  Maybe TestExpectations should know how to serialize itself back to a string and we could just print the string?

That would make sense for the --full option, but I'm not sure that it makes sense for the others; TestExpectations is more of a model than a view. 

Given that I want the script to be able to provide the different formats, if a Command isn't the right place and I don't think TestExpectations (being a model) is either, what would be recommended?

(In reply to comment #6)
> I'm not 100% sure what I'd use this command for, but it seems like a reasonable thing to do if you've got some use in mind.

The first main use is to replace skipped-files. The more common generalization is to answer questions like "which tests do we expect to crash" without having to hand-search and parse the expectations and skipped files.

[Adam Barth]

> Given that I want the script to be able to provide the different formats, if a Command isn't the right place and I don't think TestExpectations (being a model) is either, what would be recommended?

Maybe a TestExpectationsPrettyPrinter class?

[Dirk Pranke]

(In reply to comment #8)
> > Given that I want the script to be able to provide the different formats, if a Command isn't the right place and I don't think TestExpectations (being a model) is either, what would be recommended?
> 
> Maybe a TestExpectationsPrettyPrinter class?

It seems like the formats we'd print (again, apart from --full) would be pretty specific to this command-line script (i.e., there's not much potential for reuse); does it make sense to put such a class in queries.py, or somewhere else? 

Mostly since I haven't really written much webkit-patch functionality I'm trying to understand what would be consistent with your past work here ...

[Adam Barth]

> It seems like the formats we'd print (again, apart from --full) would be pretty specific to this command-line script (i.e., there's not much potential for reuse); does it make sense to put such a class in queries.py, or somewhere else? 

Serializing a set of test expectations to a string seems like a common thing that many folks might want to do.

> Mostly since I haven't really written much webkit-patch functionality I'm trying to understand what would be consistent with your past work here ...

Commands are intended to just be thin controllers around functionality in the rest of webkitpy.  They shouldn't have a detailed understanding of the objects they're manipulating.  They're just a high-level binding to command-line syntax.

[Dirk Pranke]

(In reply to comment #10)
> > It seems like the formats we'd print (again, apart from --full) would be pretty specific to this command-line script (i.e., there's not much potential for reuse); does it make sense to put such a class in queries.py, or somewhere else? 
> 
> Serializing a set of test expectations to a string seems like a common thing that many folks might want to do.
> 

Yeah, except that I'm not sure how many would want the *same* format. Like I said, the --full format might make sense as a default, but the others, not so much.

> > Mostly since I haven't really written much webkit-patch functionality I'm trying to understand what would be consistent with your past work here ...
> 
> Commands are intended to just be thin controllers around functionality in the rest of webkitpy.  They shouldn't have a detailed understanding of the objects they're manipulating.  They're just a high-level binding to command-line syntax.

Okay, I'll see what I can come up with.

[Adam Barth]

The way I would think about this command is having three stages:

1) Retrieve the expectations for a given port/platform/configuration/whatever.
2) Filter the set of expectations based on some criteria.
3) Display the filtered set of expectations.

Those three high-level tasks should be done by objects outside the Command.  The command should be in charge of translating command-line arguments into Python function calls on these objects and then shuttling data from stage 1 to 2 to 3 to stdout.

[Dirk Pranke]

Created attachment 136114 [details]
updated per review feedback

[Dirk Pranke]

I think this is a lot closer to what you have in mind ... WDYT?

[Dirk Pranke]

if this looks on-track, I'll write some tests for it and upload another patch.

[Adam Barth]

Comment on attachment 136114 [details]
updated per review feedback

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

This looks a lot better, thanks.

> Tools/Scripts/webkitpy/tool/commands/queries.py:410
> +    TYPES = set(x.upper() for x in (TestExpectations.EXPECTATIONS.keys() + TestExpectations.MODIFIERS.keys())).difference(set(['NONE']))

Can we evaluate this expression at a different time?  This will run every time webkit-patch starts up.  For example, we could make this a @memoized class function.  Actually, is this even used anywhere?

> Tools/Scripts/webkitpy/tool/commands/queries.py:420
> +             make_option('-i', '--include-keyword', action='append', default=[],

bad indent

[Dirk Pranke]

(In reply to comment #16)
> > Tools/Scripts/webkitpy/tool/commands/queries.py:410
> > +    TYPES = set(x.upper() for x in (TestExpectations.EXPECTATIONS.keys() + TestExpectations.MODIFIERS.keys())).difference(set(['NONE']))
> 
> Can we evaluate this expression at a different time?  This will run every time webkit-patch starts up.  For example, we could make this a @memoized class function.  Actually, is this even used anywhere?
> 

This wasn't even needed any more.

> > Tools/Scripts/webkitpy/tool/commands/queries.py:420
> > +             make_option('-i', '--include-keyword', action='append', default=[],
> 
> bad indent

Fixed.

[Dirk Pranke]

Created attachment 136349 [details]
add test, more review feedback, merge to head

[Adam Barth]

Comment on attachment 136349 [details]
add test, more review feedback, merge to head

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

> Tools/Scripts/webkitpy/tool/commands/queries.py:410
> +        options = [

I thought a bunch of these were shared now.

> Tools/Scripts/webkitpy/tool/commands/queries.py:498
> +
> +
> +
> +
> +

So many blank lines batman

[Dirk Pranke]

Comment on attachment 136349 [details]
add test, more review feedback, merge to head

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

>> Tools/Scripts/webkitpy/tool/commands/queries.py:410
>> +        options = [
> 
> I thought a bunch of these were shared now.

That's bug 83525, which hasn't landed yet. I'll merge them together properly.

>> Tools/Scripts/webkitpy/tool/commands/queries.py:498
>> +
> 
> So many blank lines batman

Will fix :). Thanks!

[Dirk Pranke]

Committed r113802: <http://trac.webkit.org/changeset/113802>