603012 - buildbotcustom process.factory.UnittestPackagedBuildFactory uses env=None as a default argument which does not work

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P4)

Description

[Jeff Hammel]

Attachment: + self.env.update(env or {})

Currently, UnittestPackagedBuildFactory uses the default argument `env=None`.  However, this will fail on `self.env.update(env)` in `__init__`.  This should be made `self.env.update(env or None)` or similar, or, if it is not desirable, then `env` should not be given a default argument in the function argspec.
Patch attached.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

Do you have an example of this "not working" somewhere?  http://hg.mozilla.org/build/buildbotcustom/file/0c2a2d85b511/process/factory.py#l6624 shows that we copy in the env from http://hg.mozilla.org/build/buildbotcustom/file/0c2a2d85b511/env.py first, then update with any custom env passed in to UnittestPackagedBuildFactory (with env=None in case nothing custom is passed in).

Since self.env is set before self.env.update(env) is called, I'm not sure how your patch is adding value here.  An example would be useful.

Comment 2

[Jeff Hammel]

Without the patch the following will not work:

"""
from buildbotcustom.process.factory import *

# define builder factory                                                        
f1 = UnittestPackagedBuildFactory(platform='linux',
                                  hgHost='hg.mozilla.org/',
                                  buildToolsRepoPath='build/tools',
                                  repoPath='mozilla-central',
                                  productName='firefox',
                                  test_suites=['reftest'])
"""

Since env is given a default value in the initializer, it *should* work.  The patch does no harm (if an env is given, then it will be used), so should be applied.  Alternatively, you could remove env=None in the constructor arglist and just pass env.

In any case, I see no downside to fixing this.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Attachment: make env a required arg in UnittestPackagedBuildFactory

taking out env=None, also audited misc.py for calls to UnittestPackagedBuildFactory and added the passing of env to the non-chunks version of CCTest so it matches the chunks version.  Ran test-masters on it, passed.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 483664 [details] [diff] [review]
make env a required arg in UnittestPackagedBuildFactory

http://hg.mozilla.org/build/buildbotcustom/rev/72f43b066ce8