Bug 161699

Summary: Run spindumps on timeout on iOS 9 Simulator Release WK2 (Tests)
Product: WebKit Reporter: Jonathan Bedard <jbedard>
Component: Tools / TestsAssignee: Jonathan Bedard <jbedard>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, dbates, lforschler
Priority: P2    
Version: WebKit Nightly Build   
Hardware: iPhone / iPad   
OS: iOS 9.3   
See Also: https://bugs.webkit.org/show_bug.cgi?id=161696
https://bugs.webkit.org/show_bug.cgi?id=161964
Attachments:
Description Flags
Patch
none
Patch none

Description Jonathan Bedard 2016-09-07 10:59:29 PDT
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.
Comment 1 Jonathan Bedard 2016-09-07 11:00:21 PDT
Created attachment 288162 [details]
Patch
Comment 2 Daniel Bates 2016-09-07 11:33:25 PDT
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.
Comment 3 Jonathan Bedard 2016-09-07 11:56:56 PDT
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.
Comment 4 Jonathan Bedard 2016-09-07 14:48:43 PDT
Created attachment 288190 [details]
Patch
Comment 5 Jonathan Bedard 2016-09-07 14:50:32 PDT
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.
Comment 6 Alexey Proskuryakov 2016-09-07 16:22:03 PDT
Comment on attachment 288190 [details]
Patch

Seems like concerns have been addressed, rs=me
Comment 7 Daniel Bates 2016-09-07 16:45:08 PDT
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.
Comment 8 WebKit Commit Bot 2016-09-07 17:02:39 PDT
Comment on attachment 288190 [details]
Patch

Clearing flags on attachment: 288190

Committed r205573: <http://trac.webkit.org/changeset/205573>
Comment 9 WebKit Commit Bot 2016-09-07 17:02:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Jonathan Bedard 2016-09-08 14:33:04 PDT
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.