1095058 - Handling bisection when hitting an inbound-changeset with lots of merges

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Elbart]

Occasionally I'm bisecting a problem, and I end up at a changeset which holds dozens of revisions and all kinds of merges, from m-c to m-i or from fx-team to m-i etc.

Would it be feasible to ask the user at the and of an m-i bisection to try the range on a different repo?

Comment 1

[Elbart]

E.g. https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=997d1d16fc19&tochange=ced1402861b8

That's not really helpful.

Comment 2

[Julien Pagès (:parkouss)]

Attachment: handling merges

So, based on what is described in https://parkouss.wordpress.com/2015/11/14/mozregression-new-way-for-handling-merges/, I have a proposal for that.

This is more about just handling merge commits, because it change the way a bisection can take in mozregression, by detecting which branch to switch when we find a merge. That is, we can potentially go to mozilla-inbound, fx-team, or switch back to m-c (example with pushlog url above).

For now, no fall back if that method fail. We just end the bisection if we can not determine that the commit is a merge commit. That sound fine to me. Maybe what we can work on is trying to fall back when we know it is a merge commit, but we can't determine the branch (maybe due to a typo in the commit message or whatever). Maybe in that case ask the user for a branch, or probably better do something like described in bug 1210013 comment 3 (a --resume option). Some more thinking required here.

From what I tested so far, this change is great.


Well, I would love any test and feedback, from anybody!

Comment 3

[Julien Pagès (:parkouss)]

Will, don't spend too much time on this. I mean, the more I think about it, the more I think it would require some great cleanup to handle corner cases in the worst mozregression file,

https://github.com/mozilla/mozregression/blob/master/mozregression/bisector.py

This file is really complex and somewhat poorly written (by me), and I always wanted to make things simpler in there. I guess it is the time. Not an easy task though, since this is where the mozregression magic happen :) But we would win a lot from a rewrite of this code, from maintenance to easy-to-add great next coming features. So I'll start by looking at this before trying to go further here.

Comment 4

[William Lachance (:wlach)]

Comment on attachment 8687558 [details] [review]
handling merges

Ok, I'll leave this for now. Looking forward to the next round of work. :)

Comment 5

[Julien Pagès (:parkouss)]

Comment on attachment 8687558 [details] [review]
handling merges

Ok, so I changed my mind - refactoring of the bisection module would be hard, because it is tightly coupled with the GUI. It is not that bad, we can live with it.

So, this change has a big impact.

Goodies:
 - ability to automatically switch on a new branch to bisect, based on the commit message
 - no more --inbound-branch param (branches are detected, so we can use --repo to define a repo when using dates or changesets)
 - there are some aliases for the branch names, ie m-c, central, m-i...
 - when we bisect from a branch, e.g m-c, the bisection will go as far as possible on the same branch. Previously on a day range we were going directly to m-i, but now we first reduce the range on the original branch.

Examples:

> mozregression --good-rev 96377bdbcdf3e4 --bad-rev 451a185791433bc --repo central
> mozregression --launch 0e9c6096f7e9 --repo aurora -B debug

A cleaner interface, with new possibilities, and to me a better way of doing bisection (by trying everything we can on a branch, then move to bisect on another branch if we find a clean commit message).

Drawbacks:
 - when a merge commit is not understood by mozreg, it will stop. This will leave us with a merge commit that contain our regression, so possibly hard to find the issue in there again. We can think about some ways to force using a branch on merge though, like adding a command line parameter.
 - there are some weird cases (like when a regression comes from two pushes from distincts branch that will not be handled well. I suspect that the previous flow was not that helpful for that also. We can think later of ways to at least detect that, and help the user as much as possible.

Basically, I love the change, but I can't be sure it will give better results than the current flow, because the current flow has been tested and used from quite a long time. I don't think we should keep the two approaches in mozreg, because it would be a mess in the codebase, and probably not understandable from a user point of view.

If you like the way this patch takes, we could create a mozregression 2.0, starting with this flow, and maintain in parallel for some time the mozregression 1.x branch, so users could still use the old flow in case it is more efficient - but we would also gain feedback and testing from the 2.x releases.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

FWIW, this all sounds fantastic to me. It also seems to me that this would do a better job detecting regressions for pushes that landed directly on m-c rather than going through an integration branch first (rare, but not impossible). Also, depending on how autoland goes, the long-term wish is to kill off the integration branches, so this should allow mozregression to Just Work when that happens :)

Comment 7

[William Lachance (:wlach)]

(In reply to Julien Pagès (:parkouss) from comment #5)

> Drawbacks:
>  - when a merge commit is not understood by mozreg, it will stop. This will
> leave us with a merge commit that contain our regression, so possibly hard
> to find the issue in there again. We can think about some ways to force
> using a branch on merge though, like adding a command line parameter.

I think this is ok. People will just post the regression range that they're able to get in this case. Presumably developers can continue the bisection using raw revisions if needed.

>  - there are some weird cases (like when a regression comes from two pushes
> from distincts branch that will not be handled well. I suspect that the
> previous flow was not that helpful for that also. We can think later of ways
> to at least detect that, and help the user as much as possible.

These cases seem pretty rare (if they've even happened?), but yeah, we should probably handle them. Probably the sensible thing to do is say that we couldn't find the regression on an inbound branch and fall back to just providing the nightly regression range.

> Basically, I love the change, but I can't be sure it will give better
> results than the current flow, because the current flow has been tested and
> used from quite a long time. I don't think we should keep the two approaches
> in mozreg, because it would be a mess in the codebase, and probably not
> understandable from a user point of view.
> 
> If you like the way this patch takes, we could create a mozregression 2.0,
> starting with this flow, and maintain in parallel for some time the
> mozregression 1.x branch, so users could still use the old flow in case it
> is more efficient - but we would also gain feedback and testing from the 2.x
> releases.

This seems like overkill to me. I'd try to get some feedback from a few people (RyanVM can probably suggest a few) trying a dev branch, then just switch over. If there are huge unforseen problems, we can just revert. Most people aren't going to be paying enough attention to what's going on to try different versions of mozregression.

Comment 8

[William Lachance (:wlach)]

Comment on attachment 8687558 [details] [review]
handling merges

The code looks sensible. Let me know when you're ready for a final review.

Comment 9

[Julien Pagès (:parkouss)]

Comment on attachment 8687558 [details] [review]
handling merges

Well, I think it is ready for a final review. :) Please give it a real try, we are not in a hurry to merge this.

Yeah, I agree that doing a new dev branch might be an overkill here. I just made the proposal in case people are interested for that. But as you said, we can always revert - or even create a fix branch later if we need it (I did this when I was working on the firefox oses integration). Though I'd like to update the major number of the version when we will release, as it is a big change from a usage point of view.

FYI, I just released minor versions, 1.2.0 and GUI 0.5.0 that include the latest changes.

Comment 10

[William Lachance (:wlach)]

Comment on attachment 8687558 [details] [review]
handling merges

I took a quick look, but didn't actually test the changes. I assume they work. :) Everything looks good, more or less. Some minor comments in the review.

Comment 11

[Julien Pagès (:parkouss)]

Ok, I think I tested enough. Merged everything in https://github.com/mozilla/mozregression/commit/0da2ba3d944b96ad72dae7a5bbf6071f73d452dd

I'm going to close this bug. Eventually we will face other issues more or less in the same idea, but I would prefer to open new bug(s) in that case with explicit description, given that we are probably going to handle the problem in a different way.

So, there are a few things I'd like to clean before releasing. I think last week should be ok for a new release!