[Marionette Client] Adding shuffle for tests

VERIFIED FIXED in mozilla26

Status

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: wachen, Assigned: askeing)

Tracking

unspecified
mozilla26
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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

5 years ago
Assignee: nobody → fyen
(Assignee)

Comment 1

5 years ago
Created attachment 800054 [details] [diff] [review]
bug-912890-fix.patch
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)
(Assignee)

Comment 3

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

5 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)
(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

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

5 years ago
Created attachment 800679 [details] [diff] [review]
bug-912890-fix.patch
Attachment #800054 - Attachment is obsolete: true
Attachment #800679 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2dd351292f7c
Status: NEW → RESOLVED
Last Resolved: 5 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?
(Reporter)

Comment 16

5 years ago
Bug 984208 created as needed. Also, the shuffle functionality is in marionette client alrdy! marked as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.