Bug 142094 – JSC tests should not be repeated twice for each branch builder, and should if possible have their own queue.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Matthew Mirman]

Patch forthcoming.

[Matthew Mirman]

Created attachment 247534 [details]
Adds JSC test queues

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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

[Matthew Mirman]

Created attachment 247542 [details]
Adds JSC test queues

[Matthew Mirman]

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

[Csaba Osztrogonác]

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

[Matthew Mirman]

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

[Matthew Mirman]

Created attachment 247563 [details]
Adds JSC test queues

the actual new patch.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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

[Alexey Proskuryakov]

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.

[Matthew Mirman]

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.

[Alexey Proskuryakov]

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.

[Csaba Osztrogonác]

Comment on attachment 247572 [details]
Adds JSC test queues

typo: TestAllButJSFactory -> TestAllButJSCFactory

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Comment on attachment 247658 [details]
Patch

Feel free to pick it up, fine tune, add config and upload for review.

[Csaba Osztrogonác]

Note: I added a regression test for master.cfg to make sure
we won't break any builder with refactoring - bug142166.

[Matthew Mirman]

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.

[Csaba Osztrogonác]

(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

[Matthew Mirman]

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

[Matthew Mirman]

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.

[Csaba Osztrogonác]

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

[Matthew Mirman]

Created attachment 247768 [details]
Patch

Made suggested changes.

[Csaba Osztrogonác]

Comment on attachment 247768 [details]
Patch

r=me. Please ask Lucas to push this change to the master.

[Matthew Mirman]

patch landed, closing bug. 
http://trac.webkit.org/changeset/180945