1464937 - mozregression marks bad changesets as good without any supporting evidence

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Karl Tomlinson (:karlt)]

See https://bugzilla.mozilla.org/attachment.cgi?id=8981127 for a log from
running 

~/.local/bin/mozregression --good 60 --bad b63e4bff951e

It seems to do ok narrowing the regression range to [26e53729, cfe09d01]
(using inbound builds), but then after "Switching to autoland by process of
elimination (no branch detected in commit message)", assumes that b63e4bff is
good.

Expected: don't assume b63e4bff is good.
(The command line indicated it is bad, even.)
It would be reasonable to assume that the ancestor dc845b3a8cbe,
common to 26e53729 and inbound changesets, is good.

Comment 1

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #0)
> It seems to do ok narrowing the regression range to [26e53729, cfe09d01]
> (using inbound builds)

Actually, inbound, then central.

> It would be reasonable to assume that the ancestor dc845b3a8cbe,
> common to 26e53729 and inbound changesets, is good.

I mean any ancestor common to 26e53729 and *autoland* changesets,
but either assumption would be reasonable.  It's the "ancestor of
26e53729" part that makes it a reasonable assumption.

Comment 2

[Karl Tomlinson (:karlt)]

A much simpler testcase is

% mozregression --repo autoland --good 26e53729 --bad cfe09d01
INFO: cfe09d01 is not a release, assuming it's a hash...
INFO: 26e53729 is not a release, assuming it's a hash...
INFO: Getting autoland builds between 26e53729 and cfe09d01
INFO: Testing good and bad builds to ensure that they are really good and bad...
INFO: Using local file: /home/karl/moz/mozregression/persist/e583796f1c09--autoland--target.tar.bz2
INFO: Running autoland build built on 2018-04-24 12:32:12.636000, revision e583796f
INFO: Launching /tmp/tmpLnmr0N/firefox/firefox
INFO: Application command: /tmp/tmpLnmr0N/firefox/firefox -profile /tmp/tmpf8qIjP.mozrunner
INFO: application_buildid: 20180424115203
INFO: application_changeset: e583796f1c09477e789945e22361fa7c5674e70a
INFO: application_name: Firefox
INFO: application_repository: https://hg.mozilla.org/integration/autoland
INFO: application_version: 61.0a1
Was this inbound build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry' or 'exit' and press Enter): bad
ERROR: Build was expected to be good! The initial good/bad range seems incorrect.

The last line indicates that things are obviously wrong.
The test revision e583796f from autoland has many changesets that are not in the known good revision 26e53729:
https://hg.mozilla.org/mozilla-central/log?rev=ancestors(e583796f)-ancestors(26e53729)&revcount=200

Comment 3

[William Lachance (:wlach)]

Hmm, I feel like trying to use a subcommit of a merge commit inside a regression range is perhaps something that shouldn't be supported. I feel like it would take a fair amount of additional complexity to figure out how to handle that. Perhaps we just need to detect this case and just error out.

This is just my preliminary read based on your bug report, let me know if I'm not understanding the problem correctly.

Comment 4

[Karl Tomlinson (:karlt)]

The subcommits of merge commits specified on the command line here are just push heads from other repositories.

I assumed that the branch/repo switching would use similar logic based on hashes of push heads from other repos. There is at least a difference in logic in that the command line revisions are usually re-tested for good/bad status. I don't know how else the logic differs.

The same bug exists for repo switching even when the command line revisions are push heads on the specified repo:

% mozregression --repo central --good 26e53729 --bad cfe09d01
INFO: cfe09d01 is not a release, assuming it's a hash...
INFO: 26e53729 is not a release, assuming it's a hash...
INFO: Getting mozilla-central builds between 26e53729 and cfe09d01
INFO: No more inbound revisions, bisection finished.
INFO: Last good revision: 26e53729a10976f52e75efa44e17b5e054969fec
INFO: First bad revision: cfe09d016e770c3dbb0c65aa7536db0fa5d79bbf
INFO: Pushlog:
g.mozilla.org/mozilla-central/pushloghtml?fromchange=26e53729a10976f52e75efa44e17b5e054969fec&tochange=cfe09d016e770c3dbb0c65aa7536db0fa5d79bbf

INFO: ************* Switching to autoland by process of elimination (no branch detected in commit message)
INFO: Getting autoland builds between b63e4bff951e97ce23e5673cb55a990a1bbb1a3c and cfe09d016e770c3dbb0c65aa7536db0fa5d79bbf
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b63e4bff951e97ce23e5673cb55a990a1bbb1a3c&tochange=cfe09d016e770c3dbb0c65aa7536db0fa5d79bbf

b63e4bff is assumed good, even though it contains changesets that are not in the known good revision 26e53729:
https://hg.mozilla.org/mozilla-central/log?rev=ancestors(b63e4bff)-ancestors(26e53729)&revcount=20

Comment 5

[Karl Tomlinson (:karlt)]

https://github.com/mozilla/mozregression/pull/514 fixes the originally reported issue here.

For the testcase in comment 4, the patch changes -d output from

DEBUG: Repo 'autoland' seems to have the earliest push
DEBUG: Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=a31d1d7403d7ffbc0f93e0573afadc6bc0702b0a
DEBUG: Using url: https://hg.mozilla.org/integration/autoland/json-pushes?fromchange=a31d1d7403d7ffbc0f93e0573afadc6bc0702b0a&tochange=cfe09d016e770c3dbb0c65aa7536db0fa5d79bbf
INFO: ************* Switching to autoland by process of elimination (no branch detected in commit message)
INFO: Getting autoland builds between b63e4bff951e97ce23e5673cb55a990a1bbb1a3c and cfe09d016e770c3dbb0c65aa7536db0fa5d79bbf
DEBUG: Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=b63e4bff951e97ce23e5673cb55a990a1bbb1a3c
DEBUG: Using url: https://hg.mozilla.org/integration/autoland/json-pushes?fromchange=b63e4bff951e97ce23e5673cb55a990a1bbb1a3c&tochange=cfe09d016e770c3dbb0c65aa7536db0fa5d79bbf

to

DEBUG: Repo 'autoland' seems to have the earliest push
INFO: ************* Switching to autoland by process of elimination (no branch detected in commit message)
DEBUG: Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=a31d1d7403d7ffbc0f93e0573afadc6bc0702b0a
DEBUG: Using url: https://hg.mozilla.org/integration/autoland/json-pushes?fromchange=a31d1d7403d7ffbc0f93e0573afadc6bc0702b0a&tochange=cfe09d016e770c3dbb0c65aa7536db0fa5d79bbf
DEBUG: Using url: https://hg.mozilla.org/integration/autoland/json-pushes?startID=63099&endID=63145
DEBUG: End merge handling
INFO: Getting autoland builds between dc845b3a8cbe45f88fcb0408e19f9df2e120420c and cfe09d016e770c3dbb0c65aa7536db0fa5d79bbf
DEBUG: Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=dc845b3a8cbe45f88fcb0408e19f9df2e120420c
DEBUG: Using url: https://hg.mozilla.org/integration/autoland/json-pushes?fromchange=dc845b3a8cbe45f88fcb0408e19f9df2e120420c&tochange=cfe09d016e770c3dbb0c65aa7536db0fa5d79bbf

Comment 6

[Karl Tomlinson (:karlt)]

The pull request does not address the situation in comment 2. In that situation, the safety net of testing the poor guess for a good push head saves the user from receiving an incorrect regression range. I'm not sure how intuitive it is that changeset hashes must be push heads on the default or specified repo, but that would need a separate fix.

Comment 7

[William Lachance (:wlach)]

I merged your PR, thanks!

(In reply to Karl Tomlinson (:karlt) from comment #6)

The pull request does not address the situation in comment 2. In that situation, the safety net of testing the poor guess for a good push head saves the user from receiving an incorrect regression range. I'm not sure how intuitive it is that changeset hashes must be push heads on the default or specified repo, but that would need a separate fix.

After looking over the code again, I think we would ideally fix that and it probably/hopefully wouldn't be incredibly difficult... (probably just some input checking, then querying of the hg repositories on start). Feel free to file another issue for it, or we can just wait for someone to do so when this comes up again.

Comment 8

[William Lachance (:wlach)]

P.S. I'll try to do a new release with this change next week.