Closed Bug 912890 Opened 11 years ago Closed 11 years ago

[Marionette Client] Adding shuffle for tests

Categories

(Remote Protocol :: Marionette, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla26

People

(Reporter: wachen, Assigned: askeing)

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → fyen
Attached patch bug-912890-fix.patch (obsolete) — Splinter Review
Attachment #800054 - Flags: review?
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)
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 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+
(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)
(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.
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 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+
Attachment #800054 - Attachment is obsolete: true
Attachment #800679 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2dd351292f7c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
(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?
Bug 984208 created as needed. Also, the shuffle functionality is in marionette client alrdy! marked as verified fixed.
Status: RESOLVED → VERIFIED
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: