Bug 159971 - Bots should run built-ins generator tests
Summary: Bots should run built-ins generator tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-20 06:43 PDT by youenn fablet
Modified: 2016-08-25 09:38 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.21 KB, patch)
2016-07-20 06:47 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Adding an extra step for build bots (23.26 KB, patch)
2016-07-20 09:02 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Updated according review (23.90 KB, patch)
2016-07-20 09:43 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-07-20 06:43:13 PDT
Bots should run built-ins generator tests
Comment 1 youenn fablet 2016-07-20 06:47:06 PDT
Created attachment 284100 [details]
Patch
Comment 2 Alexey Proskuryakov 2016-07-20 08:05:00 PDT
Comment on attachment 284100 [details]
Patch

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

> Tools/ChangeLog:15
> +        * Scripts/webkitpy/tool/steps/runtests.py:

It doesn't seem right to unconditionally tie these tests to run-webkit-tests. Tools that automatically process results won't know what to do with these failures.
Comment 3 youenn fablet 2016-07-20 08:48:06 PDT
> It doesn't seem right to unconditionally tie these tests to
> run-webkit-tests. Tools that automatically process results won't know what
> to do with these failures.

What additional logic is needed to actually process failing results?

I guess it may be more appropriate to add an additional step dedicated to built-in tests for build bots.
Comment 4 youenn fablet 2016-07-20 09:02:25 PDT
Created attachment 284108 [details]
Adding an extra step for build bots
Comment 5 WebKit Commit Bot 2016-07-20 09:04:54 PDT
Attachment 284108 [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 6 Alexey Proskuryakov 2016-07-20 09:27:12 PDT
Comment on attachment 284108 [details]
Adding an extra step for build bots

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

Please add this step to ProductiveSteps in BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js, otherwise the dashboard will not process failure correctly.

These tests should also run on EWS. Not something for this patch, however it is important for EWS to catch issues that can make actual bots red.

> Tools/ChangeLog:16
> +        Bots should run built-ins generator tests
> +        https://bugs.webkit.org/show_bug.cgi?id=159971

Duplicate ChangeLog.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:567
> +    name = "builtins-generation-tests"
> +    description = ["builtins-tests running"]
> +    descriptionDone = ["builtins-tests"]
> +    command = ["python", "./Tools/Scripts/run-builtins-generator-tests"]

These names are all different (builtins-generation-tests, builtins-generator-tests, builtins-tests). Can a single name be used?

I suggest matching script name, as that's the easiest.
Comment 7 Alexey Proskuryakov 2016-07-20 09:27:57 PDT
This is almost an r+, however the patch is small enough and there are enough suggested changes that I'd like to take a look at the final version before landing.
Comment 8 youenn fablet 2016-07-20 09:43:29 PDT
Created attachment 284113 [details]
Updated according review
Comment 9 WebKit Commit Bot 2016-07-20 09:45:30 PDT
Attachment 284113 [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 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 youenn fablet 2016-07-20 09:46:31 PDT
Thanks for the review.

> https://bugs.webkit.org/attachment.cgi?id=284108&action=review
> 
> Please add this step to ProductiveSteps in
> BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/
> BuildbotIteration.js, otherwise the dashboard will not process failure
> correctly.

OK

> These tests should also run on EWS. Not something for this patch, however it
> is important for EWS to catch issues that can make actual bots red.

Agreed, that was the purpose of the previous patch.
how should it be done then?

> > Tools/ChangeLog:16
> > +        Bots should run built-ins generator tests
> > +        https://bugs.webkit.org/show_bug.cgi?id=159971
> 
> Duplicate ChangeLog.

Fixed.

> > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:567
> > +    name = "builtins-generation-tests"
> > +    description = ["builtins-tests running"]
> > +    descriptionDone = ["builtins-tests"]
> > +    command = ["python", "./Tools/Scripts/run-builtins-generator-tests"]
> 
> These names are all different (builtins-generation-tests,
> builtins-generator-tests, builtins-tests). Can a single name be used?
> 
> I suggest matching script name, as that's the easiest.

Fixed.
Comment 11 Alexey Proskuryakov 2016-07-20 09:55:47 PDT
Comment on attachment 284113 [details]
Updated according review

> Agreed, that was the purpose of the previous patch. how should it be done then?

I think that EWS need to explicitly know about the various tests, so that it could report failures intelligently. For internal tool tests like this one, it is also very likely that any failures will be cross-platform, so we'll need to add logic to coalesce Bugzilla updates.
Comment 12 BJ Burg 2016-07-20 09:59:00 PDT
(In reply to comment #11)
> Comment on attachment 284113 [details]
> Updated according review
> 
> > Agreed, that was the purpose of the previous patch. how should it be done then?
> 
> I think that EWS need to explicitly know about the various tests, so that it
> could report failures intelligently. For internal tool tests like this one,
> it is also very likely that any failures will be cross-platform, so we'll
> need to add logic to coalesce Bugzilla updates.

Is there an example where this has been done already for EWS? There are many similar internal test suites (bindings tests, generator tests, etc) that will need to be hooked in the same way. So an example would be multiply-useful.
Comment 13 Alexey Proskuryakov 2016-07-20 10:24:01 PDT
There was some work started in bug 140744.
Comment 14 WebKit Commit Bot 2016-07-20 23:10:54 PDT
Comment on attachment 284113 [details]
Updated according review

Clearing flags on attachment: 284113

Committed r203489: <http://trac.webkit.org/changeset/203489>
Comment 15 WebKit Commit Bot 2016-07-20 23:10:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 youenn fablet 2016-07-21 03:22:36 PDT
Patch landed but builtin tests are not ruined.
Is there any additional thing to do?
Comment 17 Alexey Proskuryakov 2016-07-21 08:48:14 PDT
I do not know if a manual master restart is needed. Lucas would know.
Comment 18 Csaba Osztrogonác 2016-08-25 09:19:05 PDT
(In reply to comment #16)
> Patch landed but builtin tests are not ruined.
> Is there any additional thing to do?

(In reply to comment #17)
> I do not know if a manual master restart is needed. Lucas would know.

After a month bots started to run these tests and they fail on EFL/GTK bots.
Comment 19 Csaba Osztrogonác 2016-08-25 09:38:17 PDT
filed a new bug report to track Linux failures: bug161196