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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Allow multiple testvars files. v1.0

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

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 "runtests.py", line 40, in <module>
    cli()
  File "runtests.py", line 35, in cli
    runner = startTestRunner(runner_class, options, tests)
  File "runtests.py", line 19, in startTestRunner
    runner = runner_class(**vars(options))
  File "runtests.py", line 14, in __init__
    BaseMarionetteTestRunner.__init__(self, **kwargs)
  File "/Users/mdas/Code/m-c/testing/marionette/client/marionette/runner/base.py", line 527, in __init__
    self.testvars = update(self.testvars, json.loads(f.read()))
  File "/Users/mdas/Code/m-c/testing/marionette/client/marionette/runner/base.py", line 516, in update
    d[k] = update(d.get(k, {}), v)
  File "/Users/mdas/Code/m-c/testing/marionette/client/marionette/runner/base.py", line 518, in update
    d[k] = u[k]
TypeError: 'int' object does not support item assignment

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Allow multiple testvars files. v1.1

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.

Comment 4

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8517420 [details] [diff] [review]
Allow multiple testvars files. v1.1

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

works great, thanks

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1962cb07ea5

Comment 6

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3903248&repo=mozilla-inbound

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Carsten Book [:Tomcat] from comment #6)
> backed out for test failures like
> https://treeherder.mozilla.org/ui/logviewer.
> 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.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Allow multiple testvars files. v1.2

This version of the patch allows testvars to be None. Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9da9adeb47e1

Comment 9

[Malini Das [:mdas] - Away, not checking bugmail]

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!

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e541f48b3c8

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2e541f48b3c8

Comment 12

[Armen [:armenzg]]

https://hg.mozilla.org/projects/ash/rev/8fee3f35e2ba

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

In order to land bug 1109005 we need to uplift this to v2.0 and v2.1.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Uplift for mozilla-b2g32_v2_0

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

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Uplift for mozilla-b2g34_v2_1

Comment 16

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as:
mozilla-b2g34_v2_1: https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/e4139bd6f0ec
mozilla-b2g32_v2_0: https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/72c9cb4628b3

Comment 18

[Kai-Zhen Li [:seinlin][:kli]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/72c9cb4628b3