Bug 169385 – webkitpy: Add unit test to prevent breakage of EWS

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

[Jonathan Bedard]

We should have a test to prevent something like <http://trac.webkit.org/changeset/213545> from happening in the future.

[Jonathan Bedard]

Created attachment 303846 [details]
Patch

[Srinivasan Vijayaraghavan]

Comment on attachment 303846 [details]
Patch

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

Nice test! I'd suggest making more use of python's in-built types to keep the code more readable, something like below:

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:177
> +        expected_names = [
> +            'gtk-wk2-ews',
> +            'win-ews',
> +            'ios-ews',
> +            'ios-sim-ews',
> +            'mac-ews',
> +            'mac-wk2-ews',
> +            'mac-debug-ews',
> +            'mac-32bit-ews',
> +            'jsc-ews',
> +        ]
> +        classes = AbstractEarlyWarningSystem.load_ews_classes()
> +        for name in expected_names:
> +            flag = False
> +            for cls in classes:
> +                if name == cls.name:
> +                    flag = True
> +                    break
> +            if not flag:
> +                raise Exception("'{}' is not a valid EWS command but is used by EWS's infrastructure".format(name))

expected_names = set([
    'gtk-wk2-ews', 
    'gtk-wk2-ews',
    'win-ews',
    'ios-ews',
    'ios-sim-ews',
    'mac-ews',
    'mac-wk2-ews',
    'mac-debug-ews',
    'mac-32bit-ews',
    'jsc-ews',
])
classes = AbstractEarlyWarningSystem.load_ews_classes()
names = set([cls.name for cls in classes])
unexpected_names = names - expected_names
if unexpected_names:
    raise Exception("'{}' is not a valid EWS command but is used by EWS's infrastructure".format(unexpected_names.pop()))

[Srinivasan Vijayaraghavan]

Actually, since we're ultimately inheriting from unittest.TestCase, raise an AssertionError("message") instead of the generic Exception.

[Jonathan Bedard]

Created attachment 303858 [details]
Patch

[Jonathan Bedard]

Created attachment 303859 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 303859 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:157
> +        # These are the names EWS's infrastructure expects, check that they work

Can the test get the names from ews.json? Those are the ones that the infrastructure really expects.

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:169
> +        classes = AbstractEarlyWarningSystem.load_ews_classes()

Is this a persistent change that can affect other tests? Adding new globally defined classes seems less than ideal if that can be avoided.

But other EWS tests already do the same, so I think that the patch doesn't make it worse.

[Jonathan Bedard]

Comment on attachment 303859 [details]
Patch

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

>> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:157
>> +        # These are the names EWS's infrastructure expects, check that they work
> 
> Can the test get the names from ews.json? Those are the ones that the infrastructure really expects.

So this is actually exactly what is being tested here.

'load_ews_classes()' reads from ews.json and parses the names of the ports themselves and uses that combination to construct commands.  This list of commands is essentially what our EWS infrastructure currently expects to be able to run.  If we intend to change this infrastructure, this set would need to be modified.

I think this is the right way to test this, though.  It is not intuitive that changing the name of a port in webkitpy could break EWS because changing the port name will change the EWS command.

[Dean Johnson]

Comment on attachment 303859 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:158
> +        expected_names = set([

This could be constructed without a set cast by using {} (Python's set syntax). It would look like the following

expected_names = {
    'gtk-wk2-ews',
    ....,
    'jsc-ews',
}

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:170
> +        names = set([cls.name for cls in classes])

Similarly:
names = {cls.name for cls in classes}

[Jonathan Bedard]

Created attachment 303932 [details]
Patch for landing

[Srinivasan Vijayaraghavan]

I didn't suggest the set literal syntax since it's 2.7 only, but I guess the bots have all been updated now.

[WebKit Commit Bot]

Comment on attachment 303932 [details]
Patch for landing

Clearing flags on attachment: 303932

Committed r213651: <http://trac.webkit.org/changeset/213651>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.