Closed
Bug 1141118
Opened 10 years ago
Closed 6 years ago
With --shuffle option used the actual seed should be logged before the tests are run
Categories
(Remote Protocol :: Marionette, defect, P5)
Remote Protocol
Marionette
Tracking
(firefox62 fixed)
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: whimboo, Assigned: AlvaroRe, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [lang=py])
User Story
To get started on this bug please check the steps as mentioned on: https://firefox-source-docs.mozilla.org/testing/marionette/marionette/NewContributors.html
Attachments
(1 file, 3 obsolete files)
If you specify a --shuffle for the CLI the tests will be run in a specific order as set by the seed. If no seed is given a default seed is acquired. To be able to run the same order of tests again, this default seed has to be logged.
IMO it would be good to do that whenever the --shuffle option is specified.
Comment 1•10 years ago
|
||
The seed is already logged here: https://hg.mozilla.org/mozilla-central/file/eab4a81e4457/testing/marionette/client/marionette/runner/base.py#l848 as implemented by bug 984208.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 2•10 years ago
|
||
I see. The problem here was that my tests were aborted in the middle of a testrun. So I haven't seen the logging of the seed at the very end. Given that we want to re-use the seed, we should log it before the tests are run.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: With --shuffle set the actual seed should be logged → With --shuffle option used the actual seed should be logged before the tests are run
Updated•7 years ago
|
Reporter | ||
Updated•7 years ago
|
Mentor: hskupin
Comment 3•7 years ago
|
||
Hi, I'd like to take a shot at this
Reporter | ||
Comment 4•7 years ago
|
||
You are welcome to work on it. In case of questions please let us know. Otherwise have a look at the provided link in how to get started.
User Story: (updated)
Reporter | ||
Comment 6•7 years ago
|
||
Yes, you are welcome to work on this bug. Please check the user story above in how to get started. In case of questions please ask here or on irc.mozilla.org in the #ateam channel.
Flags: needinfo?(hskupin)
Comment 7•7 years ago
|
||
I tried to find the file mentioned https://hg.mozilla.org/mozilla-central/file/eab4a81e4457/testing/marionette/client/marionette/runner/base.py#l848 but it seems that the directory client/marionette no longer exists. Can you please help me in locating the file?
Flags: needinfo?(hskupin)
Comment 8•7 years ago
|
||
You can use SearchFox to search through all of central, and searching
for "seed" in testing/marionette seems to give a few good pointers
to where you need to make your change:
https://searchfox.org/mozilla-central/search?q=seed&case=false®exp=false&path=testing%2Fmarionette
Flags: needinfo?(hskupin)
Comment 9•7 years ago
|
||
(In reply to Andreas Tolfsen ‹:ato› from comment #8)
> You can use SearchFox to search through all of central, and searching
> for "seed" in testing/marionette seems to give a few good pointers
> to where you need to make your change:
>
> https://searchfox.org/mozilla-central/
> search?q=seed&case=false®exp=false&path=testing%2Fmarionette
Thank you for helping. I'll make the change.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 12•7 years ago
|
||
namrata, thank you for uploading the patch. If you think it is ready to be reviewed please make sure to open the patch in mozreview and set the reviewer flag to me. Thanks.
Assignee: nobody → namratamukhija
Status: REOPENED → ASSIGNED
Flags: needinfo?(namratamukhija)
Updated•7 years ago
|
Attachment #8959502 -
Flags: review?(hskupin)
Attachment #8959503 -
Flags: review?(hskupin)
Reporter | ||
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8959503 [details]
Bug 1141118 - Fixed logging of seed before the tests are run. The seed is now logged before the tests are run. This enables re-using of the seed, if required.
https://reviewboard.mozilla.org/r/228302/#review234994
Please use `hg histedit` to squash this extra commit into the first one. Note that usually you want to use the `--amend` option of `hg commit` to combine directly after editing.
Attachment #8959503 -
Flags: review?(hskupin)
Reporter | ||
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8959502 [details]
Bug 1141118 - Fixed logging of seed before the tests are run. The seed is now logged before the tests are run. This enables re-using of the seed, if required.
https://reviewboard.mozilla.org/r/228292/#review234996
Attachment #8959502 -
Flags: review?(hskupin)
Comment 15•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8959503 [details]
Bug 1141118 - Fixed logging of seed before the tests are run. The seed is now logged before the tests are run. This enables re-using of the seed, if required.
https://reviewboard.mozilla.org/r/228302/#review234994
Sorry, I'm new. I'll fix this right away.
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8959503 -
Attachment is obsolete: true
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8959502 [details]
Bug 1141118 - Fixed logging of seed before the tests are run. The seed is now logged before the tests are run. This enables re-using of the seed, if required.
https://reviewboard.mozilla.org/r/228292/#review234996
I have squashed the commits. Should I set you as the reviewer again?
Reporter | ||
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8959502 [details]
Bug 1141118 - Fixed logging of seed before the tests are run. The seed is now logged before the tests are run. This enables re-using of the seed, if required.
https://reviewboard.mozilla.org/r/228292/#review235732
::: commit-message-80b47:9
(Diff revision 2)
> +***
> +Bug 1141118 - Fixed logging of seed before the tests are run. The seed is now logged before the tests are run. This enables re-using of the seed, if required.
> +
> +MozReview-Commit-ID: E9ISYqNEMZA
> +
> +Bug 1141118 - Fixed logging of seed before the tests are run. The seed is now logged before the tests are run. This enables re-using of the seed, if required.
Looks like the history rewrite didn't succeed here given that you have the same line printed multiple times.
So use `hg histedit` again, and choose `m` (modify) to fix the commit message as following:
1) Use a short summary of what this change is actually modifying
2) Use a more detailed description separated by an empty line for more information.
3) Only have the summary and details listed once.
::: testing/marionette/harness/marionette_harness/runner/base.py:929
(Diff revision 2)
> - for run_tests in self.mixin_run_tests:
> - run_tests(tests)
> if self.shuffle:
> self.logger.info("Using shuffle seed: %d" % self.shuffle_seed)
> + for run_tests in self.mixin_run_tests:
> + run_tests(tests)
When you run the Marionette tests via `./mach marionette test --shuffle` you will notice that the shuffle seed will still be printed at the end of the test run. Please note that the for loop here doesn't actually run the main chunk of tests. This is done by the loop above.
So as best move the two lines up to after `self.logger.suite_start()`.
Attachment #8959502 -
Flags: review?(hskupin) → review-
Reporter | ||
Comment 19•7 years ago
|
||
Hi namrata, thank you for the patch! I checked it and added some details to the review request, which you want to get fixed. Let me know if you have troubles with histedit, and we can solve it on IRC.
Flags: needinfo?(namratamukhija)
Reporter | ||
Comment 20•6 years ago
|
||
Sadly we haven't heard from namratamukhija for about 2 months. So I'm resetting the assignee to allow others to work on it.
Assignee: namratamukhija → nobody
Status: ASSIGNED → NEW
Comment hidden (mozreview-request) |
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8974126 [details]
Bug 1141118 - Fixed logging of seed before the tests are run. The seed is now logged before the tests are run. This enables re-using of the seed, if required.
https://reviewboard.mozilla.org/r/242420/#review248338
Code analysis found 1 defect in this patch:
- 1 defect found by mozlint
You can run this analysis locally with:
- `./mach lint path/to/file` (JS/Python)
If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx
::: testing/marionette/harness/marionette_harness/runner/base.py:890
(Diff revision 1)
> self.marionette.delete_session()
>
> tests_by_group = defaultdict(list)
> for test in self.tests:
> tests_by_group[test['group']].append(test['filepath'])
> +
Error: Blank line contains whitespace [flake8: W293]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•6 years ago
|
||
Hi, I fixed those two issues in namrata's patch. Wanted to credit namratamukhija's effort but I didn't know how.
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → arb952
Status: NEW → ASSIGNED
Reporter | ||
Comment 25•6 years ago
|
||
(In reply to Alvaro Reina (:AlvaroRe) from comment #24)
> Hi, I fixed those two issues in namrata's patch. Wanted to credit
> namratamukhija's effort but I didn't know how.
Given that this is just a move of a single code block, I don't see that we have to add any credits. I will have a look at your patch in a moment.
Reporter | ||
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8974126 [details]
Bug 1141118 - Fixed logging of seed before the tests are run. The seed is now logged before the tests are run. This enables re-using of the seed, if required.
https://reviewboard.mozilla.org/r/242420/#review248470
::: commit-message-80b47:1
(Diff revision 2)
> +Bug 1141118 - Fixed logging of seed before the tests are run. The seed is now logged before the tests are run. This enables re-using of the seed, if required.
The first line of the commit message should only contain a short overview of what has been changed. The component is optional but nice to have.
So just say: "Bug 1141118 - [marionette] In shuffle mode log the seed value before the tests are run."
::: testing/marionette/harness/marionette_harness/runner/base.py:892
(Diff revision 2)
> tests_by_group = defaultdict(list)
> for test in self.tests:
> tests_by_group[test['group']].append(test['filepath'])
>
> + if self.shuffle:
> + self.logger.info("Using shuffle seed: %d" % self.shuffle_seed)
In comment 18 of the bug I mentioned to move it after the `suite_start()` call. So that it would look like:
> SUITE-START | Running 1 tests
> Using shuffle seed: 1296588404415960593
Attachment #8974126 -
Flags: review?(hskupin) → review-
Reporter | ||
Comment 27•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8974126 [details]
Bug 1141118 - Fixed logging of seed before the tests are run. The seed is now logged before the tests are run. This enables re-using of the seed, if required.
https://reviewboard.mozilla.org/r/242420/#review248338
Also please fix this linter failure.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8974126 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8975188 -
Flags: review?(hskupin)
Assignee | ||
Comment 29•6 years ago
|
||
That should be it, no more confusion between before and after.
There's a yellow line of code between revision 2 and 3 , i don't know what it means. I setted up a virtual machine for everything mozilla related after revision 2 (to get rid of Windows CRLF mainly), and I think that yellow line warns of a difference of versions, so I don't know if I screwed up again.
Reporter | ||
Comment 30•6 years ago
|
||
Comment on attachment 8959502 [details]
Bug 1141118 - Fixed logging of seed before the tests are run. The seed is now logged before the tests are run. This enables re-using of the seed, if required.
You submitted once more a new review request. I would say that before you push a patch on a different bug in the future lets have a chat on IRC so we can fix that problem. Otherwise this patch looks fine now. Thank you for the update!
Attachment #8959502 -
Attachment is obsolete: true
Reporter | ||
Comment 31•6 years ago
|
||
mozreview-review |
Comment on attachment 8975188 [details]
Bug 1141118 - [marionette] In shuffle mode log the seed value before the tests are run.
https://reviewboard.mozilla.org/r/243530/#review249518
Attachment #8975188 -
Flags: review?(hskupin) → review+
Comment 32•6 years ago
|
||
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02f1065a7bea
[marionette] In shuffle mode log the seed value before the tests are run. r=whimboo
Comment 33•6 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 6 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•