Bug 202365 – Fix iPad testers platform for downloading build product and fixing tests to run

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

[Matt Lewis]

We need to add support for device type to the testers in build automation to specify iPad OS for the new testers.

[Radar WebKit Bug Importer]

<rdar://problem/55842726>

[Matt Lewis]

This may actually be fixed by a simple config change. Making the platform to ios-simulator and adding "additionalArguments" to the queue.

[Jonathan Bedard]

Should be exactly the same as iOS, except with --ipad-simulator....although, we will need to pass --iphone-simulator to the existing queue so that iPad tests aren't duplicated.

[Matt Lewis]

So this is probably going to take a bigger change than a config change to get api test running. So in interest of getting iPad Simulator tests at least working, I'm going to focus on them. I will add in a fix for the api test in another bug/patch.

[Matt Lewis]

Created attachment 379973 [details]
Patch

[Jonathan Bedard]

(In reply to Matt Lewis from comment #4)
> So this is probably going to take a bigger change than a config change to
> get api test running. So in interest of getting iPad Simulator tests at
> least working, I'm going to focus on them. I will add in a fix for the api
> test in another bug/patch.

Can we file a bug for the bigger config change?

[Jonathan Bedard]

Comment on attachment 379973 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/config.json:198
> +                      "additionalArguments": ["--no-retry-failures", "--iphone-simulator"],

I think this will pass both --iphone-simulator and --ios-simulator to run-webkit-tests....can you verify that actually works?

[Matt Lewis]

(In reply to Jonathan Bedard from comment #7)
> Comment on attachment 379973 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379973&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/config.json:198
> > +                      "additionalArguments": ["--no-retry-failures", "--iphone-simulator"],
> 
> I think this will pass both --iphone-simulator and --ios-simulator to
> run-webkit-tests....can you verify that actually works?

Looks like no and my desire for just a config change will not be enough. I'm actually going to have to change things in steps.

[Ryan Haddad]

(In reply to Jonathan Bedard from comment #7)
> Comment on attachment 379973 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379973&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/config.json:198
> > +                      "additionalArguments": ["--no-retry-failures", "--iphone-simulator"],
> 
> I think this will pass both --iphone-simulator and --ios-simulator to
> run-webkit-tests....can you verify that actually works?
Sadly, this definitely won't work.

[Matt Lewis]

Created attachment 380164 [details]
Patch

[Jonathan Bedard]

Comment on attachment 380164 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:197
> +def appendCustomTestingFlags(step, platform, device_models):

Why is it plural?

> Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:-413
> -        appendCustomBuildFlags(self, platform, self.getProperty('fullPlatform'))

If we don't have a device model, don't we still need the property build flag?

[Matt Lewis]

(In reply to Jonathan Bedard from comment #11)
> Comment on attachment 380164 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380164&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:197
> > +def appendCustomTestingFlags(step, platform, device_models):
> 
> Why is it plural?
Shouldn't be, good catch.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:-413
> > -        appendCustomBuildFlags(self, platform, self.getProperty('fullPlatform'))
> 
> If we don't have a device model, don't we still need the property build flag?
Still working on this patch.

[Matt Lewis]

Created attachment 380181 [details]
Patch

[Jonathan Bedard]

Comment on attachment 380181 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/factories.py:30
> +    def __init__(self, platform, configuration, architectures, buildOnly, additionalArguments, SVNMirror, device_model):

Couldn't we set this to 'None' so you don't have explicitly pass it through everywhere, only in the steps that need it?

> Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:-413
> -        appendCustomBuildFlags(self, platform, self.getProperty('fullPlatform'))

Still worried that this will break other stuff. If the device_model isn't defined, we should retain the original behavior.

[Jonathan Bedard]

Comment on attachment 380181 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/factories.py:80
> +    def __init__(self, platform, configuration, architectures, additionalArguments=None, SVNMirror=None, device_models=None, device_model=None, **kwargs):

Why device_models?

[Matt Lewis]

(In reply to Jonathan Bedard from comment #14)
> Comment on attachment 380181 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380181&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/factories.py:30
> > +    def __init__(self, platform, configuration, architectures, buildOnly, additionalArguments, SVNMirror, device_model):
> 
> Couldn't we set this to 'None' so you don't have explicitly pass it through
> everywhere, only in the steps that need it?
> 
Sadly this is due to the way other properties are passed and I was conforming to existing code

> > Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:-413
> > -        appendCustomBuildFlags(self, platform, self.getProperty('fullPlatform'))
> 
> Still worried that this will break other stuff. If the device_model isn't
> defined, we should retain the original behavior.
appendCustomBuildFlags is unchanged and should still handle your worries. 

(In reply to Jonathan Bedard from comment #15)
> Comment on attachment 380181 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380181&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/factories.py:80
> > +    def __init__(self, platform, configuration, architectures, additionalArguments=None, SVNMirror=None, device_models=None, device_model=None, **kwargs):
> 
> Why device_models?
Typos are fun

[Jonathan Bedard]

(In reply to Matt Lewis from comment #16)
> (In reply to Jonathan Bedard from comment #14)
> > Comment on attachment 380181 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=380181&action=review
> > 
> > > Tools/BuildSlaveSupport/build.webkit.org-config/factories.py:30
> > > +    def __init__(self, platform, configuration, architectures, buildOnly, additionalArguments, SVNMirror, device_model):
> > 
> > Couldn't we set this to 'None' so you don't have explicitly pass it through
> > everywhere, only in the steps that need it?
> > 
> Sadly this is due to the way other properties are passed and I was
> conforming to existing code

Fair enough...could you file another bug for fixing this? Not an urgent change, but it seems like we should record it.

> > > Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:-413
> > > -        appendCustomBuildFlags(self, platform, self.getProperty('fullPlatform'))
> > 
> > Still worried that this will break other stuff. If the device_model isn't
> > defined, we should retain the original behavior.
> appendCustomBuildFlags is unchanged and should still handle your worries. 

Ok, you basically copied the the logic from that function but added model, got it.

> (In reply to Jonathan Bedard from comment #15)
> > Comment on attachment 380181 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=380181&action=review
> > 
> > > Tools/BuildSlaveSupport/build.webkit.org-config/factories.py:80
> > > +    def __init__(self, platform, configuration, architectures, additionalArguments=None, SVNMirror=None, device_models=None, device_model=None, **kwargs):
> > 
> > Why device_models?
> Typos are fun

[Matt Lewis]

Created attachment 380230 [details]
Patch

[Jonathan Bedard]

Comment on attachment 380230 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/loadConfig.py:101
> +        # print builder.get('device_model')

Nit: Extra print statement

[Matt Lewis]

Created attachment 380239 [details]
Patch

[Matt Lewis]

Fixed the nit

[Matt Lewis]

Comment on attachment 380239 [details]
Patch

Landing

[WebKit Commit Bot]

Comment on attachment 380239 [details]
Patch

Clearing flags on attachment: 380239

Committed r250746: <https://trac.webkit.org/changeset/250746>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Aakash Jain]

Comment on attachment 380239 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:204
> +    if device_model == 'ipad':

This should be 'elif' instead of 'if'.

if device_model is 'iphone', this code would set device_model to 'iphone-simulator' here, and then below 'else' statement (line 206) would be triggered as well which would (incorrectly) set it to 'ios'.