Bug 142230 – BuildAndTestFactory is redundant and needs cleaning

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Matthew Mirman]

BuildAndTestFactory in Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg contains code duplication from TestFactory, and BuildAndTestWebKit2Factory is unused.

Patch Forthcoming.

[Matthew Mirman]

Created attachment 247779 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 247779 [details]
Patch

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

Some comments added, otherwise looks good to me.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:806
>              self.addStep(RunEflAPITests)

RunEflAPITests() similar to CompileClass() below.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:-811
> -    CompileClass = CompileWebKit
> -    LayoutTestClass = RunWebKitTests
> -    ExtractTestResultsClass = ExtractTestResults

Good catch, it seems CompileClass and ExtractTestResultsClass are really useless.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:813
> +        self.addStep(CompileWebKit)

Use self.addStep(CompileWebKit()) , because passing BuildStep subclass is deprecated.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:815
> +    def __init__(self, *args, triggers=None, **kwargs):

self, *args, triggers=None, **kwargs --> self, triggers=None, *args, **kwargs

> Tools/ChangeLog:15
> +        (BuildAndTestWebKit2Factory.getProduct): Deleted.  Wasn't used anywhere.

getProduct? I know it is generated, but it can be fixed manually.

[Matthew Mirman]

(In reply to comment #2)
> > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:815
> > +    def __init__(self, *args, triggers=None, **kwargs):
> 
> self, *args, triggers=None, **kwargs --> self, triggers=None, *args, **kwargs

Is triggers being used as a keyword argument? If so, putting it at the beginning will disable any non keyword arguments being passed in.  Otherwise, if it isn't, this change doesn't work at all, since it changes the order of args.

[Matthew Mirman]

Created attachment 247785 [details]
Patch

[Matthew Mirman]

(In reply to comment #2)
> > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:815
> > +    def __init__(self, *args, triggers=None, **kwargs):
> 
> self, *args, triggers=None, **kwargs --> self, triggers=None, *args, **kwarg

I decided to just revert this.

[Csaba Osztrogonác]

Comment on attachment 247785 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:814
> +        self.addStep(CompileWebKit())
>      def __init__(self, platform, configuration, architectures, triggers=None, additionalArguments=None, SVNMirror=None, **kwargs):

please add a newline

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:815
> +        TestFactory.__init__(self, platform, configuration, architectures, False, additionalArguments, SVNMirror, **kwargs)

without False, TestFactory.__init__ has only 6 arguments.

[Matthew Mirman]

Created attachment 247798 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 247798 [details]
Patch

Rejecting attachment 247798 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 247798, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
fs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 180958 = ba5f5701f3d812202858234b65effa39dbd3461b
r180959 = 0011a228a1f200b552f03bd2fcd3b09c02fc00a3
r180960 = a0a2f39cf8dd053e9648572ffaa30ae0cf423d0d
r180961 = 06addba3300bbc41107c7a4233ed9bac2bb510bf
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.appspot.com/results/6625932274040832

[Csaba Osztrogonác]

It seems it needs to be rebased before landing.

[Csaba Osztrogonác]

(In reply to comment #9)
> It seems it needs to be rebased before landing.

Are you going to rebase and land it in the near future?

[Matthew Mirman]

(In reply to comment #10)
> (In reply to comment #9)
> > It seems it needs to be rebased before landing.
> 
> Are you going to rebase and land it in the near future?

Yes, I didn't notice this didn't get commit queued.

[Matthew Mirman]

(In reply to comment #10)
> (In reply to comment #9)
> > It seems it needs to be rebased before landing.
> 
> Are you going to rebase and land it in the near future?

(In reply to comment #10)
> (In reply to comment #9)
> > It seems it needs to be rebased before landing.
> 
> Are you going to rebase and land it in the near future?

nvm, I had already landed it, and forgot to close it.  http://trac.webkit.org/changeset/180999

[Matthew Mirman]

Comment on attachment 247798 [details]
Patch

Landed in http://trac.webkit.org/changeset/180999