Bug 159971 – Bots should run built-ins generator tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[youenn fablet]

Bots should run built-ins generator tests

[youenn fablet]

Created attachment 284100 [details]
Patch

[Alexey Proskuryakov]

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.

[youenn fablet]

> 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.

[youenn fablet]

Created attachment 284108 [details]
Adding an extra step for build bots

[WebKit Commit Bot]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[youenn fablet]

Created attachment 284113 [details]
Updated according review

[WebKit Commit Bot]

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.

[youenn fablet]

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.

[Alexey Proskuryakov]

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.

[Blaze Burg]

(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.

[Alexey Proskuryakov]

There was some work started in bug 140744.

[WebKit Commit Bot]

Comment on attachment 284113 [details]
Updated according review

Clearing flags on attachment: 284113

Committed r203489: <http://trac.webkit.org/changeset/203489>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Patch landed but builtin tests are not ruined.
Is there any additional thing to do?

[Alexey Proskuryakov]

I do not know if a manual master restart is needed. Lucas would know.

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

filed a new bug report to track Linux failures: bug161196