Patch forthcoming.
Created attachment 247534 [details] Adds JSC test queues
The unittest of the master.cfg fails: ./mastercfg_unittest.py /usr/lib/python2.7/dist-packages/twisted/spread/jelly.py:92: DeprecationWarning: the sets module is deprecated import sets as _sets Traceback (most recent call last): File "./mastercfg_unittest.py", line 447, in <module> BuildBotConfigLoader().load_config('master.cfg') File "./mastercfg_unittest.py", line 46, in load_config execfile(master_cfg_path, globals(), globals()) File "master.cfg", line 864, in <module> class TestWebKit2Factory(TestWithoutJSCTestsFactory): NameError: name 'TestWithoutJSCTestsFactory' is not defined
Comment on attachment 247534 [details] Adds JSC test queues View in context: https://bugs.webkit.org/attachment.cgi?id=247534&action=review I agree with the goal to speedup testing, but I don't like the idea to introduce the new addTestSteps and getCodeAndStart functions. I think these adds more complexity instead of simplifying the factories. I'd prefer simply adding a JSCTestClass similar to LayoutTestClass. In this case the normal full test could enable it and the AllButJSC disable it. And the JSC only tester could be a subclass of Factory and use only three steps: download, extract, runjavascriptcoretests. > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:790 > + self.addStep(ExtractBuiltProduct()) trailing whitespaces > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:793 > + ditto > Tools/ChangeLog:3 > + Added bots 155 and 157 for JSC test queues. ditto > Tools/ChangeLog:4 > + Seperated out JSC tests queues such that they only run on bots ditto > Tools/ChangeLog:6 > + Simplified and cleaned up TestFactory and BuildAndTestFactory. ditto
Created attachment 247542 [details] Adds JSC test queues
(In reply to comment #3) > Comment on attachment 247534 [details] > Adds JSC test queues > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247534&action=review > > I agree with the goal to speedup testing, but I don't like the idea to > introduce > the new addTestSteps and getCodeAndStart functions. I think these adds more > complexity instead of simplifying the factories. > I was able to remove the addTestSteps, but having getCodeAndStart allows you to remove a significant amount of code duplication between BuildAndTestFactory and TestFactory. The desired effect of removing the code duplication could in theory be achieved by squashing all of the logic into TestFactory or BuildAndTestFactory and adding object variables which turn on and off parts of it for inheriting classes, but this isn't very idiomatic python and doesn't make much organizational sense.
Comment on attachment 247542 [details] Adds JSC test queues View in context: https://bugs.webkit.org/attachment.cgi?id=247542&action=review I still don't like this idea. If it isn't super important, let me think it over until monday early morning and I'm sure we could find an ultimate cleanup. One more question: How will it affect the bot watchers dashboard? Don't we need changes in it too? > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:792 > + def getCodeAndStart(self): > + self.addStep(DownloadBuiltProduct()) > + self.addStep(ExtractBuiltProduct()) > if platform == 'win' or platform.startswith('mac'): > self.addStep(RunUnitTests()) Why RunUnitTests() belongs to getCodeAndStart() and not to addTestSteps() ? platform isn't visible here at all, it is the parameter of the constructor. > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:807 > if platform == "efl": > self.addStep(RunEflAPITests) > if platform == "gtk": > self.addStep(RunGtkAPITests()) > self.addStep(RunGtkWebKitGObjectDOMBindingsAPIBreakTests()) platform
(In reply to comment #6) > Comment on attachment 247542 [details] > Adds JSC test queues > > View in context: > https://bugs.webkit.org/attachment.cgi?id=247542&action=review > > I still don't like this idea. If it isn't super important, let me think it > over > until monday early morning and I'm sure we could find an ultimate cleanup. Yeah, thats fine. > One more question: How will it affect the bot watchers dashboard? Don't we > need changes in it too? It should, where do I go about making those changes? > > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:792 > > + def getCodeAndStart(self): > > + self.addStep(DownloadBuiltProduct()) > > + self.addStep(ExtractBuiltProduct()) > > if platform == 'win' or platform.startswith('mac'): > > self.addStep(RunUnitTests()) > > Why RunUnitTests() belongs to getCodeAndStart() and not to addTestSteps() ? Well, addTestSteps no longer exists in this patch, so it definitely doesn't belong there. Its separated out into getCodeAndStart() because BuildAndTest didn't run the unit tests, and since BuildAndTest was already going to override getCodeAndStart(), I figured putting it there was the easiest way to include it in Test but not in BuildAndTest. I've called it getCodeAndStart rather than just getCode because I do a similar trick with adding JavaScriptCore tests - I added the JavaScriptCore tests to getCodeAndStart rather than the initializer because we have a guarantee there that the build is already available. > platform isn't visible here at all, it is the parameter of the constructor. > > > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:807 > > if platform == "efl": > > self.addStep(RunEflAPITests) > > if platform == "gtk": > > self.addStep(RunGtkAPITests()) > > self.addStep(RunGtkWebKitGObjectDOMBindingsAPIBreakTests()) > > platform I realize in retrospect I uploaded the outdated patch. Apologies.
Created attachment 247563 [details] Adds JSC test queues the actual new patch.
(In reply to comment #7) > (In reply to comment #6) > > Comment on attachment 247542 [details] > > Adds JSC test queues > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=247542&action=review > > > > I still don't like this idea. If it isn't super important, let me think it > > over > > until monday early morning and I'm sure we could find an ultimate cleanup. > > Yeah, thats fine. > > > One more question: How will it affect the bot watchers dashboard? Don't we > > need changes in it too? > > It should, where do I go about making those changes? > > > > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:792 > > > + def getCodeAndStart(self): > > > + self.addStep(DownloadBuiltProduct()) > > > + self.addStep(ExtractBuiltProduct()) > > > if platform == 'win' or platform.startswith('mac'): > > > self.addStep(RunUnitTests()) > > > > Why RunUnitTests() belongs to getCodeAndStart() and not to addTestSteps() ? > > Well, addTestSteps no longer exists in this patch, so it definitely doesn't > belong there. > Its separated out into getCodeAndStart() because BuildAndTest didn't run the > unit tests, and since BuildAndTest was already going to override > getCodeAndStart(), I figured putting it there was the easiest way to include > it in Test but not in BuildAndTest. Ah, I got it. BuildAndTest didn't run unit tests because there is no mac or win BuildAndTest instance now. It's safe to move it to addTestSteps, we don't need this trick at all. This patch looks much more better, but unfortunately I don't have time now to review it in details.
(In reply to comment #7) > > One more question: How will it affect the bot watchers dashboard? Don't we > > need changes in it too? > > It should, where do I go about making those changes? It can be found in Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard , Alexey is the master of the dashboard, he can help you hacking it.
I talked to Geoff, and it's fine to take care of the dashboard in a separate step, after this patch is landed. I'd love to help with this. The complication is that we don't want to add three more green bubbles for JSC (this, plus LLINT CLOOP, plus 32-bit), so we should probably collapse these when everything passes, and show them separately when there are failures.
Created attachment 247572 [details] Adds JSC test queues Moved the unit tests back into the init. Now they won't run on the JSC only queues.
Comment on attachment 247572 [details] Adds JSC test queues View in context: https://bugs.webkit.org/attachment.cgi?id=247572&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/config.json:123 > + "additionalArguments": ["--no-retry-failures"], This isn't needed, --no-retry-failures only works for WebKit layout tests. > Tools/BuildSlaveSupport/build.webkit.org-config/config.json:151 > + "additionalArguments": ["--no-retry-failures"], Ditto. > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:801 > + def getCodeAndStart(self): I'm not excited about this - most of the time this function only prepares a built product, but sometimes, it also runs some tests. This seems confusing, I was definitely confused at first. Can we have two functions instead? One would be "prepareBuiltProduct()", and another would add extra tests.
Comment on attachment 247572 [details] Adds JSC test queues typo: TestAllButJSFactory -> TestAllButJSCFactory
Comment on attachment 247572 [details] Adds JSC test queues I think this patch is still very confusing. I suggest we should do the refactoring/simplifying and adding new features in separated patches/bugs. And I prefer as simple as possible solutions. Let's see the main goal here. We would like to have three different tester factory: 1.) run only JSC tests - TestJSCFactory 2.) run all tests except JSC tests - TestAllButJSCFactory 3.) run all tests - TestFactory
Created attachment 247658 [details] Patch In my opinion a patch like this would be the less intrusive for now. And we could do some refactoring later.
Comment on attachment 247658 [details] Patch Feel free to pick it up, fine tune, add config and upload for review.
Note: I added a regression test for master.cfg to make sure we won't break any builder with refactoring - bug142166.
Created attachment 247681 [details] Patch Took Csaba's suggestion and split this out into a simpler patch. Still made it such that JSC tests don't run on TestWebkit2 though.
(In reply to comment #19) > Created attachment 247681 [details] > Patch > > Took Csaba's suggestion and split this out into a simpler patch. Still made > it such that JSC tests don't run on TestWebkit2 though. There is one more problem, it would disable JSC tests on these bots too: - GTK Linux 64-bit Debug (Tests) - GTK Linux 64-bit Release (Tests) GTK doesn't have WebKit1 port, so the WebKit2 bots should run layout tests. What if we introduce a TestWebKit2AllButJSC for the Apple Yosemite bots and leave TestWebKit2Factory as is? class TestWebKit2AllButJSCFactory(TestFactory): LayoutTestClass = RunWebKit2Tests JSCTestClass = None
(In reply to comment #20) > (In reply to comment #19) > > Created attachment 247681 [details] > > Patch > > > > Took Csaba's suggestion and split this out into a simpler patch. Still made > > it such that JSC tests don't run on TestWebkit2 though. > > There is one more problem, it would disable JSC tests on these bots too: > - GTK Linux 64-bit Debug (Tests) > - GTK Linux 64-bit Release (Tests) > > GTK doesn't have WebKit1 port, so the WebKit2 bots should run layout tests. > What if we introduce a TestWebKit2AllButJSC for the Apple Yosemite bots > and leave TestWebKit2Factory as is? > > class TestWebKit2AllButJSCFactory(TestFactory): > LayoutTestClass = RunWebKit2Tests > JSCTestClass = None sure.
Created attachment 247687 [details] Patch Added TestWebKit2AndJSCFactory. This actually turns out to be less friction than adding TestWebKit2AllButJSCFactory - there are fewer builders that need both WebKit2 tests and JSC tests than ones that need only WebKit2 tests, and the name makes more sense.
Comment on attachment 247687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247687&action=review Overall it's good, I only suggest some minor fixes and then happy to give final r+. > Tools/BuildSlaveSupport/build.webkit.org-config/config.json:123 > + "additionalArguments": ["--no-retry-failures"], It is useless for JSC tester, additionalArguments is used for layout testing. Please remove it. > Tools/BuildSlaveSupport/build.webkit.org-config/config.json:151 > + "additionalArguments": ["--no-retry-failures"], ditto > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:880 > +class TestWebKit2Factory(TestAllButJSCFactory): > + LayoutTestClass = RunWebKit2Tests I'd prefer inheriting from TestFactory directly and adding JSCTestClass = None here too. It is only one extra line of code and easier to understand at first glance. > Tools/ChangeLog:9 > + Added bots 155 and 157 for JSC test queues. > + Seperated out JSC tests queues such that they only run on bots > + intended to test either only JSC or WK1. > + https://bugs.webkit.org/show_bug.cgi?id=142094 > + > + Reviewed by NOBODY (OOPS!). > + JSC tests should not be repeated twice for each branch builder, and should if possible have their own queue. https://bugs.webkit.org/show_bug.cgi?id=142094 Reviewed by NOBODY (OOPS!). "everything else" > Tools/ChangeLog:17 > + (TestWebKit2AndJSCFactory): Added factory to not run JSC tests on webkit2. nit: WebKit2
Created attachment 247768 [details] Patch Made suggested changes.
Comment on attachment 247768 [details] Patch r=me. Please ask Lucas to push this change to the master.
patch landed, closing bug. http://trac.webkit.org/changeset/180945