Bug 202361

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

(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

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?

(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

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

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.

Created attachment 386613 [details] Patch

Created attachment 386619 [details] Patch

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.

(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

Created attachment 387240 [details] Patch

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

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

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

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

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

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

(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

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

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

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

Created attachment 387357 [details] Patch Changes: Rename queue back to API-Tests-GTK-EWS

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

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

(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

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

Created attachment 388638 [details] Patch

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

(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

@Carlos, should we go ahead and land this?

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!

Comment on attachment 388638 [details] Patch Clearing flags on attachment: 388638 Committed r255238: <https://trac.webkit.org/changeset/255238>

All reviewed patches have been landed. Closing bug.