Bug 161699 – Run spindumps on timeout on iOS 9 Simulator Release WK2 (Tests)

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

This bug is meant to help debug https://bugs.webkit.org/show_bug.cgi?id=161696.  Spindumps are large, so they are only being selectively enabled.

[Jonathan Bedard]

Created attachment 288162 [details]
Patch

[Daniel Bates]

Comment on attachment 288162 [details]
Patch

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

> Tools/ChangeLog:3
> +        Run spindumps on timeout on iOS 9 WK 2release simulators.

"iOS 9 WK 2release simulators" => "iOS 9 Simulator Release WK2 (Tests) bots"

> Tools/ChangeLog:9
> +        (RunWebKitTests): Disable sample on timeout for everything except iOS 9 WK2 release simulators.

Do we know how much more time this may introduce for a test cycle?

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:350
> +    if WithProperties("%(buildername)s") != "Apple iOS 9 Simulator Release WK2 (Tests)":
> +        command.append("--no-sample-on-timeout")

Does this work? From my understanding the values of Buildbot properties are not available until start() is called. Also, please add a test for this change. The unit tests for Buildbot are in file <https://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py>. You can run the tests by invoking "python mastercfg_unittest.py" inside directory Tools/BuildSlaveSupport/build.webkit.org-config.

[Jonathan Bedard]

Comment on attachment 288162 [details]
Patch

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

>> Tools/ChangeLog:9
>> +        (RunWebKitTests): Disable sample on timeout for everything except iOS 9 WK2 release simulators.
> 
> Do we know how much more time this may introduce for a test cycle?

The issue won't be time (it's about 30 seconds per timeout test, so basically doubling the timeout time) it will be memory

>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:350
>> +        command.append("--no-sample-on-timeout")
> 
> Does this work? From my understanding the values of Buildbot properties are not available until start() is called. Also, please add a test for this change. The unit tests for Buildbot are in file <https://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py>. You can run the tests by invoking "python mastercfg_unittest.py" inside directory Tools/BuildSlaveSupport/build.webkit.org-config.

Well, I'm parsing the builder-name instead of the platform.  From my understanding, the builder-name is defined in the constructor, after all, we actually use the builder name in the constructor already (line 341).  I used the builder-name instead on the platform name since this change is specifically targeting a builder.

[Jonathan Bedard]

Created attachment 288190 [details]
Patch

[Jonathan Bedard]

Adding Unit Tests for this requires infrastructure we currently do not have.  Local testing has confirmed this patch, and a later patch will include Unit Tests for RunWebKitTests more generally.

[Alexey Proskuryakov]

Comment on attachment 288190 [details]
Patch

Seems like concerns have been addressed, rs=me

[Daniel Bates]

Comment on attachment 288190 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:360
> +        if WithProperties("%(buildername)s") != "Apple iOS 9 Simulator Release WK2 (Tests)":

Is there a way we can do this an avoid hardcoding some or all of the name of this bot? I mean, we will setup bots for iOS 10 soon.

On another note, it seems excessive to interpolate the value of a Buildbot property using WithProperties() as a means to gets its value. I suggest we use self.getProperty(X) to get the value directly.

[WebKit Commit Bot]

Comment on attachment 288190 [details]
Patch

Clearing flags on attachment: 288190

Committed r205573: <http://trac.webkit.org/changeset/205573>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

Comment on attachment 288190 [details]
Patch

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

>> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:360
>> +        if WithProperties("%(buildername)s") != "Apple iOS 9 Simulator Release WK2 (Tests)":
> 
> Is there a way we can do this an avoid hardcoding some or all of the name of this bot? I mean, we will setup bots for iOS 10 soon.
> 
> On another note, it seems excessive to interpolate the value of a Buildbot property using WithProperties() as a means to gets its value. I suggest we use self.getProperty(X) to get the value directly.

I'm not sure how long we intend to keep this change.  Since it is designed to catch a specific problem, I think for the time being, hardcoding is appropriate.  If we determine that these logs are useful more generally, we can use something more general.

Looking closer, yes, self.getProperty is probably a better choice and will likely be used when I update this code-path for testing.