Bug 202361 – [GTK] Add EWS testers to run GTK unit tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

With the new EWS it's now possible to add testers that only run unit tests. I think we should have one for GTK (and WPE, but we can start with GTK). It will help to keep the tests in good shape, since we usually focus only on layout tests while gardening. However, before being able to add a EWS for unit tests we need to do some previous tasks:

 - Check current status and fix failures or mark them in expectations.
 - Implement flaky tests support.

[Aakash Jain]

To be clear, which test-suite as you referring to. Is it run-api-tests for gtk platform?

[Carlos Garcia Campos]

(In reply to Aakash Jain from comment #1)
> To be clear, which test-suite as you referring to. Is it run-api-tests for
> gtk platform?

Yes, run-gtk-tests is our script. It would be the run api tests step of the buildbot

[Carlos Alberto Lopez Perez]

I think I have the patch for the buildbot/EWS ready but I wanted to give it some test... 

What is the best/current way of testing this?

I tried this but fails with an import error:

$ python Tools/BuildSlaveSupport/ews-build/runUnittests.py
E.......................
======================================================================
ERROR: steps_unittest (unittest.loader.ModuleImportFailure)
----------------------------------------------------------------------
ImportError: Failed to import test module: steps_unittest
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/loader.py", line 254, in _find_tests
    module = self._get_module_from_name(name)
  File "/usr/lib/python2.7/unittest/loader.py", line 232, in _get_module_from_name
    __import__(name)
  File "/home/clopez/webkit/webkit/Tools/BuildSlaveSupport/ews-build/steps_unittest.py", line 30, in <module>
    from buildbot.test.fake.remotecommand import Expect, ExpectRemoteRef, ExpectShell
ImportError: No module named test.fake.remotecommand


(Note that this fails even before my patch)

What versions of buildbot are expected for testing this?
Is there some python virtual environment definition somewhere that I can use for testing this?

[Aakash Jain]

(In reply to Carlos Alberto Lopez Perez from comment #3)
> What is the best/current way of testing this?
Either you can test it locally, or you can upload the patch here, and I/someone can apply to on the uat server https://ews-build.webkit-uat.org (but currently I am on vacation till dec-end)
 
> ImportError: No module named test.fake.remotecommand
Similar to https://bugs.webkit.org/show_bug.cgi?id=203899#c4 
You probably skipped passing '--no-binary :all:' to pip install command, and some of the required modules weren't installed.
You should re-install buildbot using:
sudo pip install buildbot==1.8.2 --no-binary :all:

Reference: https://trac.webkit.org/wiki/EarlyWarningSystem#Installingrequiredpackages:

> What versions of buildbot are expected for testing this?
1.8.2
> Is there some python virtual environment definition somewhere that I can use for testing this?
Nope

[Carlos Alberto Lopez Perez]

(In reply to Aakash Jain from comment #4)
> (In reply to Carlos Alberto Lopez Perez from comment #3)
> > What is the best/current way of testing this?
> Either you can test it locally, or you can upload the patch here, and
> I/someone can apply to on the uat server https://ews-build.webkit-uat.org
> (but currently I am on vacation till dec-end)
>  
> > ImportError: No module named test.fake.remotecommand
> Similar to https://bugs.webkit.org/show_bug.cgi?id=203899#c4 
> You probably skipped passing '--no-binary :all:' to pip install command, and
> some of the required modules weren't installed.
> You should re-install buildbot using:
> sudo pip install buildbot==1.8.2 --no-binary :all:
> 

That worked. Thanks!

I didn't knew about this no-binary switch.

[Carlos Alberto Lopez Perez]

It seems implementing this was more complex that I thought initially.

I see that the test step its expected to summarize the results in a json file that later is analyzed and compared to the results of the retry and the run withouth-patch.

We don't have still support in run-gtk-tests for producing this json files.. so I will try to implement that first.
I may also seize that to modify the format in which run-gtk-tests prints the summary of the results, because if we use the same format than Mac's run-api-tests we can reuse the logic already implemented for this.

[Carlos Alberto Lopez Perez]

Created attachment 386613 [details]
Patch

[Carlos Alberto Lopez Perez]

Created attachment 386619 [details]
Patch

[Aakash Jain]

Comment on attachment 386619 [details]
Patch

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

Overall looks good. Some minor comments below.

Also I have applied this patch to the UAT server https://ews-build.webkit-uat.org/#/builders/30  Can you please connect a bot to it and run some builds for testing.

> Tools/BuildSlaveSupport/ews-build/config.json:525
> +      "workernames": ["igalia4-gtk-wk2-ews"]

would one bot be enough?

> Tools/BuildSlaveSupport/ews-build/config.json:551
> +      "builderNames": ["Apply-WatchList-EWS", "Bindings-Tests-EWS", "GTK-Webkit2-EWS", "API-Tests-GTK-EWS", "iOS-13-Build-EWS", "iOS-13-Simulator-Build-EWS",

Can you please move "API-Tests-GTK-EWS" to beginning, this list is in alphabetical order.

[Carlos Alberto Lopez Perez]

(In reply to Aakash Jain from comment #9)
> Comment on attachment 386619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386619&action=review
> 
> Overall looks good. Some minor comments below.
> 
> Also I have applied this patch to the UAT server
> https://ews-build.webkit-uat.org/#/builders/30  Can you please connect a bot
> to it and run some builds for testing.
> 

I have connected it now.

> > Tools/BuildSlaveSupport/ews-build/config.json:525
> > +      "workernames": ["igalia4-gtk-wk2-ews"]
> 
> would one bot be enough?
> 

I hope. I will try to watch it after it starts running.
In case is not enough I will raise the number of cores assigned or add more workers.


> > Tools/BuildSlaveSupport/ews-build/config.json:551
> > +      "builderNames": ["Apply-WatchList-EWS", "Bindings-Tests-EWS", "GTK-Webkit2-EWS", "API-Tests-GTK-EWS", "iOS-13-Build-EWS", "iOS-13-Simulator-Build-EWS",
> 
> Can you please move "API-Tests-GTK-EWS" to beginning, this list is in
> alphabetical order.

Sure. I will update the patch

[Carlos Alberto Lopez Perez]

Created attachment 387240 [details]
Patch

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #10)
> (In reply to Aakash Jain from comment #9)
> > Comment on attachment 386619 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=386619&action=review
> > 
> > Overall looks good. Some minor comments below.
> > 
> > Also I have applied this patch to the UAT server
> > https://ews-build.webkit-uat.org/#/builders/30  Can you please connect a bot
> > to it and run some builds for testing.
> > 
> 
> I have connected it now.
> 

This test server its very useful :)

I see the bot running there its having two issues:

* The API tests step there is giving 7 extra failures that don't happen on the release buildbot.

Unexpected failures (2)
    /TestWebCore
        ComplexTextControllerTest.TotalWidthWithJustification
        ComplexTextControllerTest.InitialAdvanceInRTLNoOrigins
Unexpected timeouts (5)
    /WebKit2Gtk/TestPrinting
        /webkit/WebKitWebView/print
        /webkit/WebKitPrintOperation/close-after-print
        /webkit/WebKitPrintOperation/print-errors
        /webkit/WebKitPrintOperation/print
        /webkit/WebKitPrintOperation/custom-widget
Unexpected passes (1)
    /WebKit2Gtk/TestWebExtensions
        /webkit/WebKitWebExtension/form-submission-steps
Ran 1319 tests of 1321 with 1312 successful

Likely this is due to some different configuration of the container or something like that. Have to be investigated.

However this shouldn't be a blocker because in theory it should be able to detect this API tests were failing already before the patch, but ...

* The "unnapply patch" step its calling Tools/Scripts/clean-webkit, that wipes the JHBuild, and then its not rebuild. So the bot its being unable to properly retest without the patch applied and its unable to detect if the failures are new or not.

See: https://ews-build.webkit-uat.org/#/builders/30/builds/77

I will update this patch with a new version, trying to fix this. Ideally we should not wipe the JHBuild, since that its expensive to rebuild and ccache doesn't help much with it.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #12)
> I will update this patch with a new version, trying to fix this. Ideally we
> should not wipe the JHBuild, since that its expensive to rebuild and ccache
> doesn't help much with it.

mmm... I see the current EWS GTK/WPE build-only testers are also its doing this when trying to test without patch. 

https://ews-build.webkit.org/#/builders/4/builds/16242

The extra JHBuild step takes 30 mins.

[Carlos Alberto Lopez Perez]

Created attachment 387329 [details]
Patch

Changes: Rename queue to GTK-API-Tests-EWS. Add step to run InstallGtkDependencies() before CompileWebKitToT(). Diff with previous: http://sprunge.us/6PFKTp?diff

[Carlos Alberto Lopez Perez]

Aakash: can you apply this new version of the patch to ews-build.webkit.org? Thanks!

Now it should run the step InstallGtkDependencies() before CompileWebKitToT(), which will fix the issue (i hope)

It is still not doing the optimization of not wiping the JHBuild, but I will do that in the bug 206061

[Carlos Garcia Campos]

(In reply to Carlos Alberto Lopez Perez from comment #12)
> (In reply to Carlos Alberto Lopez Perez from comment #10)
> > (In reply to Aakash Jain from comment #9)
> > > Comment on attachment 386619 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=386619&action=review
> > > 
> > > Overall looks good. Some minor comments below.
> > > 
> > > Also I have applied this patch to the UAT server
> > > https://ews-build.webkit-uat.org/#/builders/30  Can you please connect a bot
> > > to it and run some builds for testing.
> > > 
> > 
> > I have connected it now.
> > 
> 
> This test server its very useful :)
> 
> I see the bot running there its having two issues:
> 
> * The API tests step there is giving 7 extra failures that don't happen on
> the release buildbot.
> 
> Unexpected failures (2)
>     /TestWebCore
>         ComplexTextControllerTest.TotalWidthWithJustification
>         ComplexTextControllerTest.InitialAdvanceInRTLNoOrigins
> Unexpected timeouts (5)
>     /WebKit2Gtk/TestPrinting
>         /webkit/WebKitWebView/print
>         /webkit/WebKitPrintOperation/close-after-print
>         /webkit/WebKitPrintOperation/print-errors
>         /webkit/WebKitPrintOperation/print
>         /webkit/WebKitPrintOperation/custom-widget

This is probably because cups-dev is not installed and GTK was built without cups printing support.

> Unexpected passes (1)
>     /WebKit2Gtk/TestWebExtensions
>         /webkit/WebKitWebExtension/form-submission-steps

This is falky, but I guess unexpected passes are not a problem.

> Ran 1319 tests of 1321 with 1312 successful
> 
> Likely this is due to some different configuration of the container or
> something like that. Have to be investigated.
> 
> However this shouldn't be a blocker because in theory it should be able to
> detect this API tests were failing already before the patch, but ...
> 
> * The "unnapply patch" step its calling Tools/Scripts/clean-webkit, that
> wipes the JHBuild, and then its not rebuild. So the bot its being unable to
> properly retest without the patch applied and its unable to detect if the
> failures are new or not.
> 
> See: https://ews-build.webkit-uat.org/#/builders/30/builds/77
> 
> I will update this patch with a new version, trying to fix this. Ideally we
> should not wipe the JHBuild, since that its expensive to rebuild and ccache
> doesn't help much with it.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #16)
> (In reply to Carlos Alberto Lopez Perez from comment #12)
> > (In reply to Carlos Alberto Lopez Perez from comment #10)
> > > (In reply to Aakash Jain from comment #9)
> > > > Comment on attachment 386619 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=386619&action=review
> > > > 
> > > > Overall looks good. Some minor comments below.
> > > > 
> > > > Also I have applied this patch to the UAT server
> > > > https://ews-build.webkit-uat.org/#/builders/30  Can you please connect a bot
> > > > to it and run some builds for testing.
> > > > 
> > > 
> > > I have connected it now.
> > > 
> > 
> > This test server its very useful :)
> > 
> > I see the bot running there its having two issues:
> > 
> > * The API tests step there is giving 7 extra failures that don't happen on
> > the release buildbot.
> > 
> > Unexpected failures (2)
> >     /TestWebCore
> >         ComplexTextControllerTest.TotalWidthWithJustification
> >         ComplexTextControllerTest.InitialAdvanceInRTLNoOrigins
> > Unexpected timeouts (5)
> >     /WebKit2Gtk/TestPrinting
> >         /webkit/WebKitWebView/print
> >         /webkit/WebKitPrintOperation/close-after-print
> >         /webkit/WebKitPrintOperation/print-errors
> >         /webkit/WebKitPrintOperation/print
> >         /webkit/WebKitPrintOperation/custom-widget
> 
> This is probably because cups-dev is not installed and GTK was built without
> cups printing support.
> 
> > Unexpected passes (1)
> >     /WebKit2Gtk/TestWebExtensions
> >         /webkit/WebKitWebExtension/form-submission-steps
> 
> This is flaky, but I guess unexpected passes are not a problem.

Right. I re-run install-dependencies script and it installed a bunch of packages there.

But I have now uninstalled cups-dev again as I can see how it behaves when there are API test failures and it has to re-run the step without patch.

Will install cups-dev again before moving the bot to the production EWS server.

[Aakash Jain]

(In reply to Carlos Alberto Lopez Perez from comment #15)
> Now it should run the step InstallGtkDependencies() before CompileWebKitToT(), which will fix the issue (i hope)
I belive you don't need it anymore after your change in https://bugs.webkit.org/show_bug.cgi?id=206061

[Aakash Jain]

Comment on attachment 387329 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/config.json:518
> +      "name": "GTK-API-Tests-EWS",

I liked "API-Tests-GTK-EWS" name better since it's consistent with other API-Tests queues.

> Tools/BuildSlaveSupport/ews-build/steps.py:884
> +            steps_to_add.append(InstallWpeDependencies())

You probably don't need this logic after your change in https://bugs.webkit.org/show_bug.cgi?id=206061

[Carlos Alberto Lopez Perez]

(In reply to Aakash Jain from comment #18)
> (In reply to Carlos Alberto Lopez Perez from comment #15)
> > Now it should run the step InstallGtkDependencies() before CompileWebKitToT(), which will fix the issue (i hope)
> I belive you don't need it anymore after your change in
> https://bugs.webkit.org/show_bug.cgi?id=206061

I think its still needed. Because if the patch touches the JHBuild moduleset then the JHBuild has to be wiped.

The step  InstallGtkDependencies() is a no-op if the JHBuild is already built and the moduleset has not changed since it has been built. But otherwise it wipes WebKitBuild/DependenciesGTK and rebuilds the JHBuild from scratch.

So its always needed to ensure to call InstallGtkDependencies() before calling build-webkit for ports GTK and WPE to ensure there are no issues with the third-party dependencies. In the best case this will be a no-op, but if there are modifications to the JHBuild it is the current way to ensure this modifications are up-to-date.

[Carlos Alberto Lopez Perez]

(In reply to Aakash Jain from comment #19)
> Comment on attachment 387329 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387329&action=review
> 
> > Tools/BuildSlaveSupport/ews-build/config.json:518
> > +      "name": "GTK-API-Tests-EWS",
> 
> I liked "API-Tests-GTK-EWS" name better since it's consistent with other
> API-Tests queues.
> 

Ok, no problem.. will revert it.

[Carlos Alberto Lopez Perez]

Created attachment 387357 [details]
Patch

Changes: Rename queue back to API-Tests-GTK-EWS

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #15)
> Aakash: can you apply this new version of the patch to ews-build.webkit.org?
> Thanks!
> 

ping?

[Aakash Jain]

Comment on attachment 387357 [details]
Patch

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

What's the plan to fix those 7 API test failures? Those make this queue slow.

> Tools/BuildSlaveSupport/ews-build/config.json:525
> +      "workernames": ["igalia4-gtk-wk2-ews"]

Probably we should also add "igalia3-gtk-wk2-ews" (or any other similar bot) here as well so as to share this bot on both the queues: "API-Tests-GTK-EWS" and "GTK-Webkit2-EWS".

> Tools/BuildSlaveSupport/ews-build/factories.py:180
> +        self.addStep(KillOldProcesses())

Please also add following line here:
ValidatePatch(verifyBugClosed=False, addURLs=False),

> Tools/BuildSlaveSupport/ews-build/steps.py:889
> +            self.build.addStepsAfterCurrentStep(steps_to_add)

This would add the InstallGtkDependencies step AFTER CompileWebKitToT step, not before.

To make InstallGtkDependencies step run before CompileWebKitToT, you would need to modify the addStepsAfterCurrentStep() inside ReRunAPITests() at https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-build/steps.py#L1673

[Aakash Jain]

(In reply to Aakash Jain from comment #24)
> To make InstallGtkDependencies step run before CompileWebKitToT, you would need to modify the addStepsAfterCurrentStep() inside ReRunAPITests() at https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-build/steps.py#L1673

Sample code:

-            self.build.addStepsAfterCurrentStep([UnApplyPatchIfRequired(),
-                                                ValidatePatch(verifyBugClosed=False, addURLs=False),
-                                                CompileWebKitToT(),
-                                                ValidatePatch(verifyBugClosed=False, addURLs=False),
-                                                KillOldProcesses(),
-                                                RunAPITestsWithoutPatch(),
-                                                AnalyzeAPITestsResults()])
+            steps_to_add = [UnApplyPatchIfRequired(), ValidatePatch(verifyBugClosed=False, addURLs=False)]
+            platform = self.getProperty('platform')
+            if platform == 'wpe':
+                steps_to_add.append(InstallWpeDependencies())
+            elif platform == 'gtk':
+                steps_to_add.append(InstallGtkDependencies())
+            steps_to_add.append(CompileWebKitToT())
+            steps_to_add.append(ValidatePatch(verifyBugClosed=False, addURLs=False))
+            steps_to_add.append(KillOldProcesses())
+            steps_to_add.append(RunAPITestsWithoutPatch())
+            steps_to_add.append(AnalyzeAPITestsResults())
+            # Using a single addStepsAfterCurrentStep because of https://github.com/buildbot/buildbot/issues/4874
+            self.build.addStepsAfterCurrentStep(steps_to_add)

Sample run: https://ews-build.webkit-uat.org/#/builders/30/builds/559

[Carlos Alberto Lopez Perez]

(In reply to Aakash Jain from comment #24)
> Comment on attachment 387357 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387357&action=review
> 
> What's the plan to fix those 7 API test failures? Those make this queue slow.
> 

I wanted to see how the bot behaves when it encounters failures, so I left deleted there on purpose cups-dev that causes failures. I will fix that before moving the bot to the real EWS queue.

However, I'm seeing we are now getting some failures not related with cups. We would try to fix them

> > Tools/BuildSlaveSupport/ews-build/config.json:525
> > +      "workernames": ["igalia4-gtk-wk2-ews"]
> 
> Probably we should also add "igalia3-gtk-wk2-ews" (or any other similar bot)
> here as well so as to share this bot on both the queues: "API-Tests-GTK-EWS"
> and "GTK-Webkit2-EWS".
> 

Yes, that makes sense.

> > Tools/BuildSlaveSupport/ews-build/factories.py:180
> > +        self.addStep(KillOldProcesses())
> 
> Please also add following line here:
> ValidatePatch(verifyBugClosed=False, addURLs=False),
> 
> > Tools/BuildSlaveSupport/ews-build/steps.py:889
> > +            self.build.addStepsAfterCurrentStep(steps_to_add)
> 
> This would add the InstallGtkDependencies step AFTER CompileWebKitToT step,
> not before.
> 
> To make InstallGtkDependencies step run before CompileWebKitToT, you would
> need to modify the addStepsAfterCurrentStep() inside ReRunAPITests() at
> https://trac.webkit.org/browser/webkit/trunk/Tools/BuildSlaveSupport/ews-
> build/steps.py#L1673

I see. 
Thanks a lot for the comments! very helpful :)

I will upload an updated the patch now, it will be very helpful if you can deploy it on ews-build.webkit-uat.org to test it and see if it behaves as expected (as well to investigate that there are no unexpected api test failures on the bot)

[Carlos Alberto Lopez Perez]

Created attachment 388638 [details]
Patch

[Aakash Jain]

Comment on attachment 388638 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/config.json:-318
> -      "workernames": ["igalia1-gtk-wk2-ews", "igalia2-gtk-wk2-ews", "igalia3-gtk-wk2-ews", "igalia4-gtk-wk2-ews", "aperez-gtk-ews"]

Do you want to remove "igalia3-gtk-wk2-ews" from GTK-Webkit2-EWS queue. Would 2 bots be sufficient for this queue? You have the option of keeping a bot in both the queues, and share the bots on both queues (e.g.: ews151, ews119 are shared on multiple queues).

[Aakash Jain]

(In reply to Carlos Alberto Lopez Perez from comment #26)
> (In reply to Aakash Jain from comment #24)
> I will upload an updated the patch now, it will be very helpful if you can
> deploy it on ews-build.webkit-uat.org to test it and see if it behaves as
> expected (as well to investigate that there are no unexpected api test failures on the bot)
Most of it was already deployed. That's how I got the sample run in https://bugs.webkit.org/show_bug.cgi?id=202361#c25

[Aakash Jain]

@Carlos, should we go ahead and land this?

[Carlos Alberto Lopez Perez]

Yes, will land it now(In reply to Aakash Jain from comment #28)
> Comment on attachment 388638 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388638&action=review
> 
> > Tools/BuildSlaveSupport/ews-build/config.json:-318
> > -      "workernames": ["igalia1-gtk-wk2-ews", "igalia2-gtk-wk2-ews", "igalia3-gtk-wk2-ews", "igalia4-gtk-wk2-ews", "aperez-gtk-ews"]
> 
> Do you want to remove "igalia3-gtk-wk2-ews" from GTK-Webkit2-EWS queue.
> Would 2 bots be sufficient for this queue? You have the option of keeping a
> bot in both the queues, and share the bots on both queues (e.g.: ews151,
> ews119 are shared on multiple queues).

Yes, lets move it to this new queue and see how things go.
We can add a new builder later or share one between queues if its not enough computing power.

Do you know where I can check average build wait times in the queue?


(In reply to Aakash Jain from comment #30)
> @Carlos, should we go ahead and land this?

Yes, let's land it.

Thanks!

[Carlos Alberto Lopez Perez]

Comment on attachment 388638 [details]
Patch

Clearing flags on attachment: 388638

Committed r255238: <https://trac.webkit.org/changeset/255238>

[Carlos Alberto Lopez Perez]

All reviewed patches have been landed.  Closing bug.