Add an option to trychooser to select --bisect-chunk options

NEW
Unassigned

Status

Release Engineering
Tools
3 years ago
a year ago

People

(Reporter: vaibhav1994, Unassigned)

Tracking

({trychooser})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/1999] )

Attachments

(5 attachments, 5 obsolete attachments)

(Reporter)

Description

3 years ago
We have the code for bisection/ignoring intermittents ready, and we could use the TryChooser to provide an option to enable it.
We would like to have some try syntax that developers can use to suppress intermittents:
try -b do -p all -u all -t none --no_random

when this flag (we can call it whatever we want) is set, we would want to add "--bisect-chunk default" to the mochitest command line.
(Reporter)

Comment 2

3 years ago
Created attachment 8476893 [details] [diff] [review]
mozharness-configure.patch

As discussed in the IRC, I wanted to add an option for --bisect-chunk to trychooser. This has to be done only for desktop mochitests. :jlund, I would like your feedback regarding it. Thanks.
Attachment #8476893 - Flags: feedback?(jlund)
(Reporter)

Comment 3

3 years ago
Created attachment 8476895 [details] [diff] [review]
trychooser.patch

The patch of including --no-random (for --bisect-chunk) in trychooser.

Comment 4

3 years ago
Comment on attachment 8476893 [details] [diff] [review]
mozharness-configure.patch

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

I think this should achieve what you want. lgtm

btw I am on PTO till wed so I will not be able to review before that

::: scripts/desktop_unittest.py
@@ +278,5 @@
>              if c.get('total_chunks') and c.get('this_chunk'):
>                  base_cmd.extend(['--total-chunks', c['total_chunks'],
>                                   '--this-chunk', c['this_chunk']])
>  
> +            if c['no_random'] and suite_category == "mochitest":

maybe we should log if (c['no_random'] and suite_cat is not mochitest). That way we don't silently ignore --no-random?
Attachment #8476893 - Flags: feedback?(jlund) → feedback+
(Reporter)

Comment 5

3 years ago
Created attachment 8477565 [details] [diff] [review]
mozharness-configure.patch

Made the changes in the patch suggested by :jlund. Since :jlund is on pto, :Callek will you please review this patch? :)
Attachment #8476893 - Attachment is obsolete: true
Attachment #8477565 - Flags: review?(bugspam.Callek)
(Reporter)

Comment 6

3 years ago
Created attachment 8477568 [details]
trychooser.png

This is how the trychooser looks after the front-end patch.
(Reporter)

Comment 7

3 years ago
Comment on attachment 8476895 [details] [diff] [review]
trychooser.patch

The front-end patch for the trychooser. Attached an image showing how it looks.
Attachment #8476895 - Flags: review?(bugspam.Callek)
Comment on attachment 8476895 [details] [diff] [review]
trychooser.patch

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

Sorry its taken me a long time, I have PTO starting tomorrow and unlikely to get to it today, Ryan can review this patch
Attachment #8476895 - Flags: review?(bugspam.Callek) → review?(ryanvm)
Comment on attachment 8477565 [details] [diff] [review]
mozharness-configure.patch

...and I'm told jordan is back today, since he has state here figure its best for him to review this one
Attachment #8477565 - Flags: review?(bugspam.Callek) → review?(jlund)
I'm sorry, I'm lacking context here. What does ignoring intermittents mean?
Comment on attachment 8477565 [details] [diff] [review]
mozharness-configure.patch

lgtm :)

can we test this out on cypress? basically land it on default branch. Cypress uses default so you can re-trigger a job or two via tbpl on cypress. Any job that goes through desktop_unittest.py. e.g. linux/mac/win mochi/ref/xpcshell tests.

https://wiki.mozilla.org/ReleaseEngineering/SpecialBranches#Cypress for more details.

ensuring that this does not break anything for existing suites on cypress will be a nice check before the next time mozharness default merges to production (production being everywhere)
Attachment #8477565 - Flags: review?(jlund) → review+
(Reporter)

Comment 12

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> I'm sorry, I'm lacking context here. What does ignoring intermittents mean?

Ryan, I had done work over the summer to bisect mochitests, as you may already know. Now, using that way, we can identify whether a failing test is intermittent or not. If it is an intermittent failure, we silently discard it and show up the job as green on tbpl. So I have added a try-chooser option so that one can currently choose to activate this process. The patch adds a word('--no-random') in the try-chooser syntax which then sets the flag in mozharness to activate the process. The patch that I have put up for you to review, adds that word to the try-chooser.
(In reply to Vaibhav (:vaibhav1994) from comment #12)
> If it is an intermittent failure, we silently discard
> it and show up the job as green on tbpl. 

How many times will we retrigger the failure? ie: would we trigger 4 times, only get green on the last run and call it green? It's just that we do have "almost perma-orange" intermittents, that would be good to avoid slipping through try :-)
Assignee: nobody → vaibhavmagarwal
Status: NEW → ASSIGNED
(Reporter)

Comment 14

3 years ago
(In reply to Jordan Lund (:jlund) from comment #11) 
> can we test this out on cypress? basically land it on default branch.
> Cypress uses default so you can re-trigger a job or two via tbpl on cypress.
> Any job that goes through desktop_unittest.py. e.g. linux/mac/win
> mochi/ref/xpcshell tests.
> ensuring that this does not break anything for existing suites on cypress
> will be a nice check before the next time mozharness default merges to
> production (production being everywhere)

Jordan, we are using this only for "try" servers for developers to use and test, do we need to still test on cypress?
Flags: needinfo?(jlund)
Comment on attachment 8476895 [details] [diff] [review]
trychooser.patch

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

r=me

Per our IRC discussion, I think we should set the threshold to 10% to start (we can always loosen it if we find it's too noisy). And we need to TinderboxPrint the tests that got retriggered. Even if it's not fatal for the run, I'm sure we'll find ourselves wanting visibility into the retriggers when investigating new intermittents that appear in production.

::: trychooser/index.html
@@ +199,5 @@
>              </div>
>  
>          </div> <!-- col1 div -->
>          <div id="col2">
> +            <h4>Ignore Intermittents</h4>

I think that we need to be more descriptive about what this is doing. Maybe instead of "Ignore Intermittents", we should say "Auto-retrigger Failing Tests"? Then a description under it along the lines of:

To avoid known intermittents, this option retriggers failing tests and marks the run green if the failure rate is <10%. Retriggered tests will be noted in the TBPL/Treeherder job info pane.

@@ +200,5 @@
>  
>          </div> <!-- col1 div -->
>          <div id="col2">
> +            <h4>Ignore Intermittents</h4>
> +            <label><input type="checkbox" class="intermittent">Yes (for linux, mac, windows mochitest only)</label>

Can we just say (desktop platforms only)?
Attachment #8476895 - Flags: review?(ryanvm) → review+
(Reporter)

Comment 16

3 years ago
Created attachment 8480581 [details] [diff] [review]
changethreshold.patch

Based on Ryan's recommendation, I have changed the threshold limit for an intermittent from 20% to 10%.
Attachment #8480581 - Flags: review?(jmaher)

Updated

3 years ago
Attachment #8480581 - Flags: review?(jmaher) → review+
(Reporter)

Comment 17

3 years ago
Created attachment 8480615 [details] [diff] [review]
trychooser.patch

Did some changes as pointed out by Ryan.
Attachment #8476895 - Attachment is obsolete: true
Attachment #8477568 - Attachment is obsolete: true
Attachment #8480615 - Flags: review?(ryanvm)
(Reporter)

Comment 18

3 years ago
Created attachment 8480616 [details]
trychooser.png

The front-end of try-chooser looks like this.
Comment on attachment 8480615 [details] [diff] [review]
trychooser.patch

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

Do we want to use a <label> here vs a <p>?
Attachment #8480615 - Flags: review?(ryanvm) → review+
Perhaps also s/Auto retrigger/Auto re-run/ (or "retry").
Retrigger to me (and likely others used to the current situation) sounds like "buildbot retrigger of job where we end up with two full job icons displaying in TBPL/Treeherder).
(For the trychooser heading and description)
(Reporter)

Comment 22

3 years ago
Created attachment 8480657 [details] [diff] [review]
trychooser.patch

Fixed some nits. Carrying forward the r+
Attachment #8480615 - Attachment is obsolete: true
Attachment #8480616 - Attachment is obsolete: true
Attachment #8480657 - Flags: review+
(Reporter)

Comment 23

3 years ago
Created attachment 8480662 [details]
trychooser.png

Front-end look of try-chooser.
(Reporter)

Comment 24

3 years ago
Created attachment 8480669 [details] [diff] [review]
tinderboxprint.patch

This patch will print to TinderboxPrint if an intermittent occurs.
Attachment #8480669 - Flags: review?(jmaher)
Comment on attachment 8480669 [details] [diff] [review]
tinderboxprint.patch

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

with my limited understanding of TinderboxPrint, this looks awesome.  Lets see how this look in try server, we can adjust it if needed!
Attachment #8480669 - Flags: review?(jmaher) → review+
(In reply to Vaibhav (:vaibhav1994) from comment #14)
> (In reply to Jordan Lund (:jlund) from comment #11) 
> > can we test this out on cypress? basically land it on default branch.
> > Cypress uses default so you can re-trigger a job or two via tbpl on cypress.
> > Any job that goes through desktop_unittest.py. e.g. linux/mac/win
> > mochi/ref/xpcshell tests.
> > ensuring that this does not break anything for existing suites on cypress
> > will be a nice check before the next time mozharness default merges to
> > production (production being everywhere)
> 
> Jordan, we are using this only for "try" servers for developers to use and
> test, do we need to still test on cypress?

I see what you mean but we have to land this on mozharness's default branch anyway. Release Engineering will merge it to production periodically. While we wait for the prod merge, you will just need to trigger a couple tests on cypress. I can't see any issues from this patch but it is good practice to negative test and make sure that it doesn't affect existing jobs outside of its intended use case?

If there was say a typo we missed and it broke every single unittest across all our branches, that would be bad :)

Does that make sense?
Flags: needinfo?(jlund)
Comment on attachment 8477565 [details] [diff] [review]
mozharness-configure.patch

in production: http://hg.mozilla.org/build/mozharness/rev/0e4582e4b857
Attachment #8477565 - Flags: checked-in+

Updated

3 years ago
Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/1991]

Updated

3 years ago
Whiteboard: [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/1991] → [kanban:engops:https://mozilla.kanbanize.com/ctrl_board/6/1999]
(Reporter)

Comment 28

2 years ago
Don't think I would be working on it anytime soon, clearing the assigned flag.
Assignee: vaibhavmagarwal → nobody
Status: ASSIGNED → NEW
Keywords: trychooser
You need to log in before you can comment on or make changes to this bug.