Bug 207648 – [GTK] Add EWS testers to run GTK layout tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Diego Pino]

[GTK] Add EWS testers to run GTK layout tests

[Diego Pino]

Created attachment 390538 [details]
Patch

[Aakash Jain]

Are these tests green enough to be run in EWS? 

I see ~50 layout test failures on https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29?numbuilds=200

[Aakash Jain]

Comment on attachment 390538 [details]
Patch

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

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

We already have a buildOnly queue for GTK. We should simply change that to buildAndTest, instead of having one queue for only building, and another one for building+testing.

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

typo in capitalization: 'GTK-Webkit2' -> 'GTK-WebKit2'

> Tools/BuildSlaveSupport/ews-build/factories.py:196
> +class GTKTestsFactory(GTKBuildAndTestFactory):

This seems like a misleading name, this factory is BuildAndTest, not just Test. Maybe something like GTKWebKitBuildAndTestFactory might be more appropriate.

[Carlos Alberto Lopez Perez]

Comment on attachment 390538 [details]
Patch

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

>> Tools/BuildSlaveSupport/ews-build/config.json:554
>> +      "name": "GTK-WebKit2-Tests-EWS",
> 
> We already have a buildOnly queue for GTK. We should simply change that to buildAndTest, instead of having one queue for only building, and another one for building+testing.

We need a build-only queue for GTK. We want three bubbles for GTK: "gtk", "gtk-wk2" and "api-gtk"
in r255238 when I added the queue for API-Tests-GTK-EWS I added it as build+test because of concerns related with having to upload many megabytes to internet up and down for each patch.
but now that I check this more carefully I see current release built-products are only 67MB for GTK release. That's few data by today's standards.. so maybe I should have made it test-only.
So I see two options:
1) add this new queue as build+test like the API-Tests-GTK-EWS one, and then on a next patch try to switch this two queues to test-only.
2) add directly this new queue as test-only and then try to switch the API-Tests-GTK-EWS one.

My recommendation right now would be to do 1, because making the bots test-only has a few tricks for GTK like:
A) the jhbuild step has to run always before the download step (the jhbuild libraries are not sent as part of the built product)
B) on the bot testing you have to deploy symbolic links for each path of the build-only machine as the built-product sent its not relocatable.
In other words if "aperez-gtk-ews" builds in /home/ews-user/worker/GTK-Webkit2-EWS/build and "igalia5-gtk-wk3-ews" builds in /home/ews/worker/GTK-WebKit2-Tests-EWS/build you need a symbolic link in "igalia5-gtk-wk3-ews" from /home/ews-user/worker/GTK-Webkit2-EWS/build to /home/ews/worker/GTK-WebKit2-Tests-EWS/build and the same for every other build-only worker that can send a built product there.

My recommendation right now would be to start with 1) "make this new queue build+test" to not have to deal with extra complexity in the deployment and then when we have things stable enough look in a next patch to try to switch both queues to test-only

[Aakash Jain]

(In reply to Carlos Alberto Lopez Perez from comment #4)
> > We already have a buildOnly queue for GTK. We should simply change that to buildAndTest, instead of having one queue for only building, and another one for building+testing.
> 
> We need a build-only queue for GTK. We want three bubbles for GTK: "gtk", "gtk-wk2" and "api-gtk"
I am not clear why we need standalone build-only queue? If seems like it's easier for you guys to have both gtk-wk2 and api-gtk as buildAndTest queues. That's fine given that gtk is very fast to build.

So if gtk-wk2 (or api-gtk) queue is already doing build+test, isn't it redundant to have a separate queue doing only build?

[Carlos Alberto Lopez Perez]

(In reply to Aakash Jain from comment #5)
> (In reply to Carlos Alberto Lopez Perez from comment #4)
> > > We already have a buildOnly queue for GTK. We should simply change that to buildAndTest, instead of having one queue for only building, and another one for building+testing.
> > 
> > We need a build-only queue for GTK. We want three bubbles for GTK: "gtk", "gtk-wk2" and "api-gtk"
> I am not clear why we need standalone build-only queue? If seems like it's
> easier for you guys to have both gtk-wk2 and api-gtk as buildAndTest queues.
> That's fine given that gtk is very fast to build.
> 
> So if gtk-wk2 (or api-gtk) queue is already doing build+test, isn't it
> redundant to have a separate queue doing only build?

I don't think its redundant because "gtk-wk2" or "api-gtk" can be failing due to failing to pass tests (or because of flakies), but "gtk" can be passing (because it builds).

Its the same than for mac or ios: we have "mac", "api-mac" and "mac-wk1" and "mac-wk2"

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #6)
> (In reply to Aakash Jain from comment #5)
> > (In reply to Carlos Alberto Lopez Perez from comment #4)
> > > > We already have a buildOnly queue for GTK. We should simply change that to buildAndTest, instead of having one queue for only building, and another one for building+testing.
> > > 
> > > We need a build-only queue for GTK. We want three bubbles for GTK: "gtk", "gtk-wk2" and "api-gtk"
> > I am not clear why we need standalone build-only queue? If seems like it's
> > easier for you guys to have both gtk-wk2 and api-gtk as buildAndTest queues.
> > That's fine given that gtk is very fast to build.
> > 
> > So if gtk-wk2 (or api-gtk) queue is already doing build+test, isn't it
> > redundant to have a separate queue doing only build?
> 
> I don't think its redundant because "gtk-wk2" or "api-gtk" can be failing
> due to failing to pass tests (or because of flakies), but "gtk" can be
> passing (because it builds).
> 
> Its the same than for mac or ios: we have "mac", "api-mac" and "mac-wk1" and
> "mac-wk2"

Also I think we should be looking forward to make "gtk-wk2" and "api-gtk" test-only instead of build+test. But I would do that after adding this new queue.

[Aakash Jain]

(In reply to Carlos Alberto Lopez Perez from comment #6)
> I don't think its redundant because "gtk-wk2" or "api-gtk" can be failing due to failing to pass tests (or because of flakies), but "gtk" can be passing (because it builds).
Take a look at windows queue, if there is a build or test failure, the error indicates which one it is. 
e.g.:
"Patch does not build" in https://ews-build.webkit.org/#/builders/10/builds/7016
"Found 1 new test failure:" in https://ews-build.webkit.org/#/builders/10/builds/6803
 
> Its the same than for mac or ios: we have "mac", "api-mac" and "mac-wk1" and "mac-wk2"
None of them have duplication of builds, they either only build or only test. The primary reason for separation was to avoid re-building.

> Also I think we should be looking forward to make "gtk-wk2" and "api-gtk" test-only instead of build+test.
That's another valid approach as well.

> But I would do that after adding this new queue.
I would prefer that to be done upfront, else there is a risk of this becoming a very low priority task and getting delayed significantly.
Also I believe addressing concerns in Comment 2 might take a while.

[Carlos Alberto Lopez Perez]

(In reply to Aakash Jain from comment #8)
> (In reply to Carlos Alberto Lopez Perez from comment #6)
> > I don't think its redundant because "gtk-wk2" or "api-gtk" can be failing due to failing to pass tests (or because of flakies), but "gtk" can be passing (because it builds).
> Take a look at windows queue, if there is a build or test failure, the error
> indicates which one it is. 
> e.g.:
> "Patch does not build" in
> https://ews-build.webkit.org/#/builders/10/builds/7016
> "Found 1 new test failure:" in
> https://ews-build.webkit.org/#/builders/10/builds/6803
>  
> > Its the same than for mac or ios: we have "mac", "api-mac" and "mac-wk1" and "mac-wk2"
> None of them have duplication of builds, they either only build or only
> test. The primary reason for separation was to avoid re-building.
> 
> > Also I think we should be looking forward to make "gtk-wk2" and "api-gtk" test-only instead of build+test.
> That's another valid approach as well.
> 
> > But I would do that after adding this new queue.
> I would prefer that to be done upfront, else there is a risk of this
> becoming a very low priority task and getting delayed significantly.
> Also I believe addressing concerns in Comment 2 might take a while.

Ok. Fair enough. Then I think we should add the new "gtk-wk2" queue as test-only.

But I would like to have a "gtk" bubble that only builds and its fast to become green without having to wait for layout tests for that.

Perhaps it even make sense to try to move the current gtk-api one to test-only first to see how that goes, don't know.

[Diego Pino]

About the current status of layout test failures, we're working on bringing that number to zero or near zero.

If I grab the last 5 reports and intersect the results to check what errors have been consistently failing the number of failures at this moment is much lower than 50 (8 actually). All the other failures are flaky failures that are coming mainly from media related tests. It seems since mid-december several media related tests became flaky (webkit.org/b/198830). We're looking into it before marking the tests as flaky.

[Diego Pino]

Created attachment 393123 [details]
Patch

[Diego Pino]

Created attachment 393125 [details]
Patch

[Diego Pino]

I'm retaking this patch since now the number of LayoutTests failures is almost 0 https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20(Tests)?numbuilds=50 

I addressed comments on last review (bot name convention and set bot as testOnly bot).

[Aakash Jain]

Comment on attachment 393125 [details]
Patch

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

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

This isn't the right scheduler for a tester. 
1) A new Triggerable should be added (just like ios-13-sim-wk2-tests-ews), e.g.: gtk-webkit2-tests-ews 

2) The builder ('GTK-Webkit2-EWS' in this case) which want to trigger this tester should have "triggers": ["gtk-webkit2-tests-ews"]

> Tools/BuildSlaveSupport/ews-build/factories.py:198
>  

inside GTKBuildFactory() skipUpload should be False (or removed since default is False).

> Tools/BuildSlaveSupport/ews-build/factories.py:200
> +    LayoutTestClass = RunWebKitTests

This doesn't seems to take care of concerns in https://bugs.webkit.org/show_bug.cgi?id=207648#c4

[Diego Pino]

Created attachment 393138 [details]
Patch

[Diego Pino]

After talking with Carlos, we agreed on switching the current API Tests EWS bot to testOnly before going deeper with this patch.

We will change the API Tests EWS bot in a different bug. After this change, we expect this patch will become much simpler and easier to review.

[Carlos Alberto Lopez Perez]

(In reply to Diego Pino from comment #16)
> After talking with Carlos, we agreed on switching the current API Tests EWS
> bot to testOnly before going deeper with this patch.
> 
> We will change the API Tests EWS bot in a different bug. After this change,
> we expect this patch will become much simpler and easier to review.

which is bug 208860

[Diego Pino]

Created attachment 393283 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 393283 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/config.json:368
> +    {
> +      "name": "GTK-WK2-Tests-EWS",
> +      "shortname": "gtk-wk2",
> +      "icon": "testOnly",
> +      "factory": "GTKTestsFactory",
> +      "platform": "gtk",
> +      "configuration": "release",
> +      "architectures": ["x86_64"],
> +      "triggers": ["gtk-webkit2-tests-ews"],

This is wrong.
You need to set triggers in the queue named "GTK-Build-EWS".
Look for example the queue named "macOS-Mojave-Release-Build-EWS" how it triggers "triggers": ["api-tests-mac-ews", "macos-mojave-release-wk1-tests-ews", "macos-mojave-release-wk2-tests-ews"],
In our case the queue ""GTK-Build-EWS" needs to trigger "GTK-WK2-Tests-EWS" and "API-Tests-GTK-EWS"

[Diego Pino]

Created attachment 393466 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 393466 [details]
Patch

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

r=me. Patch looks fine.
But I have a couple of suggestions, so its nice if you can take them into account before landing.

> Tools/ChangeLog:5
> +        [GTK] Add EWS testers to run GTK layout tests
> +        https://bugs.webkit.org/show_bug.cgi?id=207648
> +

I miss general comments on your changelogs. Something as simple as:
"The queue GTK-Webkit2-EWS its now renamed to GTK-Build-EWS and we add a new queue for WK2 Tests (layout tests) named GTK-Webkit2-EWS"
Makes easier understand the goal of the patch for those taking a look at it

> Tools/BuildSlaveSupport/ews-build/config.json:319
> +      "name": "GTK-Build-EWS",

There was an unit-test testing for this name that it discovered by grepping the old name on the code.
We should update the test now that the name its gone. Proposed update: http://sprunge.us/k3EfbI

[Diego Pino]

Created attachment 393479 [details]
Patch

[Aakash Jain]

Comment on attachment 393479 [details]
Patch

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

we would also need to add the statusbubble, but we can do it in separate patch once we deploy this and this seems to be working fine.

Might be a good idea to add unit-test in factories_unittest.py (although we haven't added lot of unit-tests there, it might be a good idea to do that). Simply add a new class similar to TestCommitQueueFactory.

> Tools/BuildSlaveSupport/ews-build/config.json:360
> +    {

let's move this next to GTK-Build-EWS section.

> Tools/BuildSlaveSupport/ews-build/config.json:632
> +      "name": "gtk-webkit2-tests-ews",

let's rename this to gtk-wk2-tests-ews to maintain consistency with other similar Triggerable.

[Diego Pino]

Created attachment 393495 [details]
Patch

[Diego Pino]

Created attachment 393502 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 393502 [details]
Patch

Clearing flags on attachment: 393502

Committed r258418: <https://trac.webkit.org/changeset/258418>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60427812>

[Aakash Jain]

Restarted master to pick up this change. New workers seems to be offline though (worker password needs to be updated on the bots).