Bug 140637 – Fix EWS python unit tests and address code comments as per 140476

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

[Jake Nielsen]

Fix EWS python unit tests and address code comments as per 140476

[Jake Nielsen]

Created attachment 244920 [details]
Patch

[Jake Nielsen]

Martin/Carlos, is the default architecture in ews.json correct for GTK?
Gyuyoung, is the default correct for EFL?

[Daniel Bates]

Comment on attachment 244920 [details]
Patch

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

I'm unclear why we need logic for the default architecture as opposed to letting the tools that the EWS call (e.g. build-webkit) determine the default architecture. Is it not sufficient to only teach webkit-patch and the EWS to pass the specified architecture (given as a command line argument when you start the EWS) to the tools is calls? I mean, it seems weird that we have logic to determine the default architecture for the EWS code given that we have existing logic to compute a default architecture in webkitdirs.pm, which is used by build-webkit and other Perl tools.

> Tools/Scripts/webkitpy/common/config/ews.json:4
> +        "architecture": "x86",

We'll need to confirm this.

> Tools/Scripts/webkitpy/common/config/ews.json:11
> +        "architecture": "x86",

Ditto.

> Tools/Scripts/webkitpy/common/config/ews.json:18
> +        "architecture": "x86"

I'm surprised we don't have 64-bit hardware.

> Tools/Scripts/webkitpy/port/base.py:108
> +        if not self.get_option('architecture'):
> +            self.set_option('architecture', self.DEFAULT_ARCHITECTURE)

We can simplify this code by taking advantage of the convenience function set_option_default() that conditionally sets the value of the specified attribute to the specified value if it doesn't already have a value.

self.set_option_default('architecture', self.DEFAULT_ARCHITECTURE)

> Tools/Scripts/webkitpy/tool/commands/queues.py:77
> +        if not hasattr(self, 'architecture'):

How did you come to the decision to check for the existence of the attribute architecture as opposed to setting it directly? I take it you're worried about the order of calling super() in a derived class?

[Jake Nielsen]

(In reply to comment #3)
> Comment on attachment 244920 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244920&action=review
> 
> I'm unclear why we need logic for the default architecture as opposed to
> letting the tools that the EWS call (e.g. build-webkit) determine the
> default architecture. Is it not sufficient to only teach webkit-patch and
> the EWS to pass the specified architecture (given as a command line argument
> when you start the EWS) to the tools is calls? I mean, it seems weird that
> we have logic to determine the default architecture for the EWS code given
> that we have existing logic to compute a default architecture in
> webkitdirs.pm, which is used by build-webkit and other Perl tools.
> 
> > Tools/Scripts/webkitpy/common/config/ews.json:4
> > +        "architecture": "x86",
> 
> We'll need to confirm this.
> 
> > Tools/Scripts/webkitpy/common/config/ews.json:11
> > +        "architecture": "x86",
> 
> Ditto.
> 
> > Tools/Scripts/webkitpy/common/config/ews.json:18
> > +        "architecture": "x86"
> 
> I'm surprised we don't have 64-bit hardware.

^ I'll remove these from ews.json

> 
> > Tools/Scripts/webkitpy/port/base.py:108
> > +        if not self.get_option('architecture'):
> > +            self.set_option('architecture', self.DEFAULT_ARCHITECTURE)
> 
> We can simplify this code by taking advantage of the convenience function
> set_option_default() that conditionally sets the value of the specified
> attribute to the specified value if it doesn't already have a value.
> 
> self.set_option_default('architecture', self.DEFAULT_ARCHITECTURE)

I agree, that is simpler. Unfortunately we now need a state variable, so we're going to need to check for the existence of the architecture option anyway.

> 
> > Tools/Scripts/webkitpy/tool/commands/queues.py:77
> > +        if not hasattr(self, 'architecture'):
> 
> How did you come to the decision to check for the existence of the attribute
> architecture as opposed to setting it directly? I take it you're worried
> about the order of calling super() in a derived class?

Yes. In particular when the EarlyWarningSystem classes are dynamically created, they are created with the architecture attribute before the initializer runs. Without this check they get set to None erroneously.

[Jake Nielsen]

Created attachment 244944 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 244944 [details]
Patch

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

r=me with the suggested fixes.

> Tools/Scripts/webkitpy/port/base.py:104
> +        # This member variable shouldn't have to exist, but it will be needed until the correct
> +        # default architectures for the GTK and EFL EWS bots are known.
> +        # FIXME: Remove this member variable.

Please reword this as:

# FIXME: This can be removed once default architectures for GTK and EFL EWS bots are set.

> Tools/Scripts/webkitpy/port/base.py:105
> +        self.did_override_architecture = True

Please set this to False by default, and only set it to true when the setter is called.

> Tools/Scripts/webkitpy/port/base.py:113
> +            self.did_override_architecture = False

Remove this line if self.did_override_architecture defaults to False.

> Tools/Scripts/webkitpy/port/ios.py:49
> +    VERSION_FALLBACK_ORDER = ['ios-7', 'ios-8', 'ios-9']

Remove 'ios-9' here since we don't need a fallback for ios-9 until ios-10 exists.

> Tools/Scripts/webkitpy/port/ios.py:50
> +    DEFAULT_ARCHITECTURE = 'armv7'

Nit: Put this after ARCHITECTURES but before VERSION_FALLBACK_ORDER.

> Tools/Scripts/webkitpy/tool/commands/queues.py:282
> +        self._port.set_architecture(self.architecture)

Should probably only set this if self.architecture is not None?

    if self.architecture:
        self._port.set_architecture(self.architecture)

[Jake Nielsen]

Created attachment 245015 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 245015 [details]
Patch

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

r=me

> Tools/Scripts/webkitpy/port/ios.py:50
> +    VERSION_FALLBACK_ORDER = ['ios-7', 'ios-8', 'ios-9']

Talked to Jake in person; it's fine to add this for now.

[WebKit Commit Bot]

Comment on attachment 245015 [details]
Patch

Clearing flags on attachment: 245015

Committed r178757: <http://trac.webkit.org/changeset/178757>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.