Bug 161816 - Fix mastercfg_unittest
Summary: Fix mastercfg_unittest
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-09 14:37 PDT by Jonathan Bedard
Modified: 2016-09-14 08:18 PDT (History)
2 users (show)

See Also:


Attachments
Patch (31.15 KB, patch)
2016-09-09 14:50 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (21.91 KB, patch)
2016-09-13 16:07 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (21.72 KB, patch)
2016-09-13 16:36 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2016-09-09 14:37:57 PDT
Changes brought on by https://bugs.webkit.org/show_bug.cgi?id=161699 and increased debugging capabilities on build-bots.
Comment 1 Jonathan Bedard 2016-09-09 14:50:35 PDT
Created attachment 288443 [details]
Patch
Comment 2 WebKit Commit Bot 2016-09-09 14:51:45 PDT
Attachment 288443 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:368:  whitespace before ':'  [pep8/E203] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jonathan Bedard 2016-09-09 14:55:29 PDT
Note that EWS is incorrect to flag the ':' in this case (based on the format of this and other files).  Also note that given the circumstances we use --no-sample-on-timeout, it fits better in config.json, especially since the future intent is to run --no-sample-on-timeout for all platforms.
Comment 4 Jonathan Bedard 2016-09-13 15:07:19 PDT
Comment on attachment 288443 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/config.json:-212
> -                      "triggers": ["ios-simulator-9-release-tests-wk1", "ios-simulator-9-release-tests-wk2"],

This is definitely wrong, will be updated shortly
Comment 5 Daniel Bates 2016-09-13 15:28:18 PDT
Comment on attachment 288443 [details]
Patch

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

> Tools/ChangeLog:3
> +        Fix mastercfg_unittest, move --no-sample-on-timeout to config.json

r-, please separate out the fixed in this patch into separate bugs. This patch contains at least three bug fixes: 1) fix the unit tests (*) 2) Use additionalArguments to pass "--no-sample-on-timeout" to the test runner instead of hardcoding this behavior with respect to the bot "Apple iOS 9 Simulator Release WK2 (Tests)" and 3) change the triggers for "Apple iOS 9 Simulator Release (Build)" (though you implies this change may be incorrect). It is preferred that each bug/patch contains one fix. This allows each bug to serve a forum for a single fix making more straightforward to follow the conversation and iterate on a fix. It also has the advantage that if there is bug in one part of a patch that we do not need to revert the entire patch instead we can revert the bad part (since it will be a patch associated with its own bug).

(*) I thought we were going to do this before we would land the patch for bug #161699 based on our in-person conversation on 09/07; I am saddened that we did not.
Comment 6 Jonathan Bedard 2016-09-13 15:53:22 PDT
(In reply to comment #5)
> Comment on attachment 288443 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288443&action=review
> 
> ...
>

Based on what you have said, I think that this patch needs to be 2 bugs, not 3.  The trigger change which I noted was a typo on my part which I noticed earlier today and quickly made note of instead of uploading a change.

I had intended to separate the unit tests from bug #161699 because I originally attempted to test build-bot properties, until coming to the conclusion that this would have required more infrastructure than would be worth it, and included the small unit test fix in this patch.  Sorry I didn't communicate that plan well!
Comment 7 Jonathan Bedard 2016-09-13 16:07:43 PDT
Created attachment 288742 [details]
Patch
Comment 8 Jonathan Bedard 2016-09-13 16:08:18 PDT
This new patch addresses only the fixes to the master config unit tests.
Comment 9 WebKit Commit Bot 2016-09-13 16:09:42 PDT
Attachment 288742 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:368:  whitespace before ':'  [pep8/E203] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Jonathan Bedard 2016-09-13 16:36:35 PDT
Created attachment 288745 [details]
Patch
Comment 11 WebKit Commit Bot 2016-09-13 16:38:20 PDT
Attachment 288745 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:368:  whitespace before ':'  [pep8/E203] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Daniel Bates 2016-09-13 16:45:24 PDT
Comment on attachment 288745 [details]
Patch

Thank you for keeping this bug focused on fixing the unit tests and for fixing the unit tests.
Comment 13 WebKit Commit Bot 2016-09-14 08:18:11 PDT
Comment on attachment 288745 [details]
Patch

Clearing flags on attachment: 288745

Committed r205904: <http://trac.webkit.org/changeset/205904>
Comment 14 WebKit Commit Bot 2016-09-14 08:18:15 PDT
All reviewed patches have been landed.  Closing bug.