Closed Bug 1089536 Opened 8 years ago Closed 8 years ago

Allow multiple test variable files to be specified on the command line


(Testing :: Marionette, defect)

Not set


(b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Tracking Status
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed


(Reporter: davehunt, Assigned: davehunt)




(3 files, 2 obsolete files)

At the moment we have duplicate data across our test variable files. There was talk recently of adding Marketplace credentials to these, so I think it makes sense to allow multiple files to be specified. As the test variable dictionary could contain nested dictionaries we would need to perform a deep update in order to merge the files.
Comment on attachment 8511864 [details] [diff] [review]
Allow multiple testvars files. v1.0

Review of attachment 8511864 [details] [diff] [review]:

Something isn't quite right with the update function. If I have one file, testvars.json, containing:

{"asdf": 3}

and another, testvars2.json, containing:

{"asdf": {"ee":3}, "asdfe":3}

Then if I run with --testvars=testvars.json --testvars=testvars2.json, I get the following error:

  File "", line 40, in <module>
  File "", line 35, in cli
    runner = startTestRunner(runner_class, options, tests)
  File "", line 19, in startTestRunner
    runner = runner_class(**vars(options))
  File "", line 14, in __init__
    BaseMarionetteTestRunner.__init__(self, **kwargs)
  File "/Users/mdas/Code/m-c/testing/marionette/client/marionette/runner/", line 527, in __init__
    self.testvars = update(self.testvars, json.loads(
  File "/Users/mdas/Code/m-c/testing/marionette/client/marionette/runner/", line 516, in update
    d[k] = update(d.get(k, {}), v)
  File "/Users/mdas/Code/m-c/testing/marionette/client/marionette/runner/", line 518, in update
    d[k] = u[k]
TypeError: 'int' object does not support item assignment
Attachment #8511864 - Flags: review?(mdas) → review-
This was because an integer value was attempted to be updated by a dictionary. I've updated the patch so that if the original value is not a dictionary but the new value is, the new value takes precedent.
Attachment #8511864 - Attachment is obsolete: true
Attachment #8517420 - Flags: review?(mdas)
Comment on attachment 8517420 [details] [diff] [review]
Allow multiple testvars files. v1.1

Review of attachment 8517420 [details] [diff] [review]:

works great, thanks
Attachment #8517420 - Flags: review?(mdas) → review+
(In reply to Carsten Book [:Tomcat] from comment #6)
> backed out for test failures like
> html#?job_id=3903248&repo=mozilla-inbound

Sorry about that, I realised immediately after pushing that I didn't run on Try first, so I was watching the push on Treeherder. Thanks for backing it out Tomcat, I'm running a new patch on Try now.
This version of the patch allows testvars to be None. Try:
Attachment #8517420 - Attachment is obsolete: true
Attachment #8522987 - Flags: review?(mdas)
Comment on attachment 8522987 [details] [diff] [review]
Allow multiple testvars files. v1.2

Review of attachment 8522987 [details] [diff] [review]:

Heh whoops, I should have tested that case as well :/ Thanks for the fix!
Attachment #8522987 - Flags: review?(mdas) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
In order to land bug 1109005 we need to uplift this to v2.0 and v2.1.
Blocks: 1109005
Resolution: FIXED → ---
Malini: Do you know if there's anything I need to do to get this approved for the release branches? I've not done an uplift for quite a while...
Attachment #8535650 - Flags: review?(mdas)
Comment on attachment 8535650 [details] [diff] [review]
Uplift for mozilla-b2g32_v2_0

Review of attachment 8535650 [details] [diff] [review]:

ah sorry, I was pto that day. You can just land as a=test-only, you don't need additional r?s so long as the patch doesn't have to be heavily modified to land.
Attachment #8535650 - Flags: review?(mdas) → review+
Attachment #8535674 - Flags: review?(mdas) → review+
Landed as:
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.