Closed
Bug 912890
Opened 11 years ago
Closed 11 years ago
[Marionette Client] Adding shuffle for tests
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla26
People
(Reporter: wachen, Assigned: askeing)
Details
Attachments
(1 file, 1 obsolete file)
6.16 KB,
patch
|
askeing
:
review+
|
Details | Diff | Splinter Review |
We are doing a new stability test in Taiwan. We need a way to shuffle all the tests in marionette client. We want to do a low risk patch to add shuffle function.
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → fyen
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #800054 -
Flags: review?
Comment 2•11 years ago
|
||
I believe this has come up before, to randomise the order of tests. It may make sense to add this enhancement directly to manifestdestiny so other testrunners can take advantage of it. What do you think, Jeff?
Flags: needinfo?(jhammel)
Assignee | ||
Comment 3•11 years ago
|
||
However the testruner will only can shuffle tests from manifest.ini file, and can not directly shuffle test files and tests files in the folder. Would it be better if we modify both manifestdestiny and marionette-client?
Comment 4•11 years ago
|
||
Comment on attachment 800054 [details] [diff] [review] bug-912890-fix.patch >+ target_tests = sorted(target_tests, key=lambda *args: random.random()) >+ #for i in manifest.get(tests=manifest_tests, **testargs): >+ for i in target_tests: Can we remove the commented line from the patch. This change looks like it might be useful but defering to :jgriffin as module owner for r? as I am not sure if he wants this feature where it is.
Attachment #800054 -
Flags: review?(jgriffin)
Attachment #800054 -
Flags: review?
Attachment #800054 -
Flags: feedback+
Comment 5•11 years ago
|
||
(In reply to Dave Hunt (:davehunt) from comment #2) > I believe this has come up before, to randomise the order of tests. It may > make sense to add this enhancement directly to manifestdestiny so other > testrunners can take advantage of it. What do you think, Jeff? I would add this functionality to ManifestDestiny if desired; however, I'm not sure its really needed. Since e.g. get returns the desired tests, it isn't much to shuffle after that: import random tests = manifest.get() random.shuffle(tests) I'd rather have this sort of thing be up to the harness vs. manifestdestiny but it isn't a strong opinion.
Flags: needinfo?(jhammel)
Comment 6•11 years ago
|
||
(In reply to Jeff Hammel [:jhammel] from comment #5) > I'd rather have this sort of thing be up to the harness vs. manifestdestiny > but it isn't a strong opinion. Fair enough, and apparently other harnesses have a --shuffle option already.
Comment 7•11 years ago
|
||
If moztest becomes a reality, I would say that it is the appropriate place for this; though again, the opinion isn't strong and my opinion may change depending on how implementation goes.
Comment 8•11 years ago
|
||
Comment on attachment 800054 [details] [diff] [review] bug-912890-fix.patch Review of attachment 800054 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Please use random.shuffle() as Jeff suggests, it's a little more concise and readable than sorted(...). ::: testing/marionette/client/marionette/runtests.py @@ +514,5 @@ > > + target_tests = manifest.get(tests=manifest_tests, **testargs) > + if self.shuffle: > + target_tests = sorted(target_tests, key=lambda *args: random.random()) > + #for i in manifest.get(tests=manifest_tests, **testargs): Please delete this line.
Attachment #800054 -
Flags: review?(jgriffin) → review+
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #800054 -
Attachment is obsolete: true
Attachment #800679 -
Flags: review+
Assignee | ||
Comment 10•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a4e94186e6a4
Assignee | ||
Comment 11•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=80aec804c54e
Keywords: checkin-needed
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/2dd351292f7c
Keywords: checkin-needed
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2dd351292f7c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 14•11 years ago
|
||
How about set the random seed explicitly before calling shuffle() ? In this way, we could keep the seed (print it out) and repeat the same sequence if we need.
Comment 15•10 years ago
|
||
(In reply to Szu-Yu Chen [:aknow] from comment #14) > How about set the random seed explicitly before calling shuffle() ? > In this way, we could keep the seed (print it out) and repeat the same > sequence if we need. You should create a new bug for this. Looks like this request got lost?
Reporter | ||
Comment 16•10 years ago
|
||
Bug 984208 created as needed. Also, the shuffle functionality is in marionette client alrdy! marked as verified fixed.
Status: RESOLVED → VERIFIED
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•