Bots should run built-ins generator tests
Created attachment 284100 [details] Patch
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.
> 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.
Created attachment 284108 [details] Adding an extra step for build bots
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 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.
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.
Created attachment 284113 [details] Updated according review
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.
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 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.
(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.
There was some work started in bug 140744.
Comment on attachment 284113 [details] Updated according review Clearing flags on attachment: 284113 Committed r203489: <http://trac.webkit.org/changeset/203489>
All reviewed patches have been landed. Closing bug.
Patch landed but builtin tests are not ruined. Is there any additional thing to do?
I do not know if a manual master restart is needed. Lucas would know.
(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.
filed a new bug report to track Linux failures: bug161196