1641263 - Fixed by commit should permit marking commits that are not the tipmost of a push

Status: NEW

(Tree Management :: Treeherder: Frontend, defect)

Description

[Armen [:armenzg]]

Attachment: image.png

marco pointed out that when trying to marked a job by "fixed by commit" we only show in the drop down the tip-most revision of a push. Unfortunately, it's frequent to backout in one push more than one regresion. This means that certain jobs that get fixed by a commit are referring to the wrong commit, thus, polutting the data the ML is using.

camd, sclementes: Would it difficult to show all commits in view? What code would need changing?

This should be fixed as part of the smart scheduling initiative. I will track it in the kanban board.

More info from mozci issue (I added some links to Treeherder to make it easier to follow along):

If a failure is marked as fixed by commit of a backout push which contains multiple backouts, we can't really know which of the backouts is actually relevant.

From Matrix:

marco: Aryx: I'm often seeing 'fixed by commit' information pointing to the wrong commit when a backout push contains two backouts

Aryx: yes, because TH only offers the push head

marco: ah, so that means it's consistent. I wonder if I can find an automated way to figure it out

Aryx: I copy the correct commit if I can mark all in one go, but if the trickle in, that's really cumbersome where it started and if the failing test got touched will get you close. The backout commit messages also mention the failing test or a part of the failure message (e.g. assert)

marco: Unfortunately it's not easy to know where it started, unless the failures are all backfilled (but afaics they are not always) looking at the logs to get the failure message might be expensive, unless that's available in ActiveData (I'll ask ekyle)

For example, a0d10a7e5c3f219e8346667b107cb02506cbbdd8 backs out both 48a854ee6710ee77fce1b7dc08799352d6ff3fdb and 379e2c6c049115d64977587961d08b42a09ad3c9.

build-linux64-fuzzing/debug is due to 379e2c6c049115d64977587961d08b42a09ad3c9, while build-android-x86_64-asan-fuzzing/opt is due to 48a854ee6710ee77fce1b7dc08799352d6ff3fdb, but they:

• are both classified as fixed by commit a0d10a7e5c3f219e8346667b107cb02506cbbdd8;
• both only run and failed in 48a854ee6710ee77fce1b7dc08799352d6ff3fdb.

The backout of 48a854ee6710ee77fce1b7dc08799352d6ff3fdb is b45a70689704ffc67766812444530cb15005eb45 (which is part of the a0d10a7e5c3f219e8346667b107cb02506cbbdd8 push).

Comment 1

[Cameron Dawson [:camd]]

That dropdown is just a shortcut to add the commit sha to the comment field. You could copy and paste any sha in the comment field that you want. That being said, we could change the call that populates that dropdown to include everything in view. I don't know the specifics of how to get that off the top of my head, but it's all in-memory. So you could glean it from there. We could also add a field where we collect them to the pushes.js Redux store. But the list would probably get pretty long. Seems easier to just copy and paste it if it's not one of the tip revisions.

Comment 2

[Marco Castelluccio [:marco]]

(In reply to Cameron Dawson [:camd] from comment #1)

That dropdown is just a shortcut to add the commit sha to the comment field. You could copy and paste any sha in the comment field that you want. That being said, we could change the call that populates that dropdown to include everything in view. I don't know the specifics of how to get that off the top of my head, but it's all in-memory. So you could glean it from there. We could also add a field where we collect them to the pushes.js Redux store. But the list would probably get pretty long. Seems easier to just copy and paste it if it's not one of the tip revisions.

Aryx mentioned this as one of the reasons why sheriffs pick the wrong commit for 'fixed by commit' (when there is a high number of failures, they don't copy and paste).
Another possible solution would be to remove the dropdown completely, so sheriffs have to copy and paste.
Yet another possibility would be to list all commits, but filter out the ones that can't possibly have fixed the failure (pushes that come before the failure). I think we should do this nevertheless to avoid bad classifications.