1106804 - --inbound-branch should imply --inbound

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Elbart]

It took me longer than I'd like to admit to find out why the following commandline wasn't working:
> $ mozregression --persist /c/temp --bits=32 --profile e10s --inbound-branch=fx-
team --good-rev=88adcf8fef83 --bad-rev=6bd2071b373f

Isn't it redundant to require --inbound when --inbound-branch is used?

Comment 1

[Elbart]

https://github.com/mozilla/mozregression/blob/de68d01f6b1a20fd616806a320e2369c141e2347/mozregression/regression.py#L406

Changing
> if options.inbound:
to
> if options.inbound or options.inbound_branch:
would work.

Comment 2

[William Lachance (:wlach)]

I agree, this should be a fairly easy change.

Comment 3

[Vaibhav (:vaibhav1994)]

Attachment: patch

This patch fixes the problem.

Comment 4

[William Lachance (:wlach)]

Comment on attachment 8532187 [details] [review]
patch

Sorry, I changed my mind. :) Looking at this a bit more, I think we might actually want to remove the '--inbound' option entirely, and bisect inbound initially if --good-rev and --bad-rev are both set. Could you update your patch accordingly? It should be a minor change.

Comment 5

[Vaibhav (:vaibhav1994)]

Comment on attachment 8532187 [details] [review]
patch

Updated to remove --inbound

Comment 6

[Elbart]

--good-rev/--bad-rev aren't setting options.inbound_branch.
Using --good-rev/--bad-rev without --inbound-branch/--inbound should bisect mozilla-inbound by default, if I understand comment 4 correctly.

Comment 7

[Sam Garrett]

(In reply to Vaibhav (:vaibhav1994) from comment #5)
> Comment on attachment 8532187 [details] [review]
> patch
> 
> Updated to remove --inbound

Can you update your patch according to comment 4? Specifically when good and bad-rev are set it would bisect.

Comment 8

[William Lachance (:wlach)]

Comment on attachment 8532187 [details] [review]
patch

Yes, see feedback in comment 6 and comment 7.

Comment 9

[Vaibhav (:vaibhav1994)]

I am busy the next 2 days with exams, will correct it this week.

Comment 10

[Vaibhav (:vaibhav1994)]

Attachment: patch

Added the default parameter for --inbound-branch to "mozilla-inbound", so that it bisects in case where only --good-rev and --bad-rev is given.

Comment 11

[William Lachance (:wlach)]

Comment on attachment 8533842 [details] [review]
patch

Unfortunately this patch still doesn't quite do what's required, see the github comments.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Vaibhav, can you take another look at this and see if you can solve this issues in the github comments?

Comment 13

[Vaibhav (:vaibhav1994)]

I tried pulling the latest source code and running it. I am getting this error:

(venv)vaibhav@vaibhav:~/mozregression$ mozregression --bits=32 --profile e10s --inbound-branch=fx-team --inbound --good-rev=88adcf8fef83 --bad-rev=6bd2071b373f
 0:00.00 LOG: MainThread Bisector INFO Getting inbound builds between 88adcf8fef83 and 6bd2071b373f
 0:17.04 LOG: MainThread Test Runner INFO Testing inbound build with timestamp 1414495907, revision f26373e6
 0:17.04 LOG: MainThread Test Runner INFO Downloading build from: http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/fx-team-linux/1414495907/firefox-36.0a1.en-US.linux-i686.tar.bz2
===== Downloaded 100% =====
['/tmp/tmpS6MJQz/firefox/firefox', '-profile', '/home/vaibhav/mozregression/e10s']
Traceback (most recent call last):
  File "/home/vaibhav/mozregression/venv/bin/mozregression", line 9, in <module>
    load_entry_point('mozregression==0.33', 'console_scripts', 'mozregression')()
  File "/home/vaibhav/mozregression/venv/local/lib/python2.7/site-packages/mozregression/main.py", line 258, in cli
    sys.exit(bisect(runner, logger))
  File "/home/vaibhav/mozregression/venv/local/lib/python2.7/site-packages/mozregression/main.py", line 162, in bisect_inbound
    options.first_bad_revision)
  File "/home/vaibhav/mozregression/venv/local/lib/python2.7/site-packages/mozregression/bisector.py", line 403, in bisect_inbound
    range=60*60*12)
  File "/home/vaibhav/mozregression/venv/local/lib/python2.7/site-packages/mozregression/bisector.py", line 250, in bisect
    return self._bisect(handler, build_data)
  File "/home/vaibhav/mozregression/venv/local/lib/python2.7/site-packages/mozregression/bisector.py", line 274, in _bisect
    allow_back=bool(previous_data))
  File "/home/vaibhav/mozregression/venv/local/lib/python2.7/site-packages/mozregression/test_runner.py", line 131, in evaluate
    launcher.start(**self.launcher_kwargs)
  File "/home/vaibhav/mozregression/venv/local/lib/python2.7/site-packages/mozregression/launchers.py", line 66, in start
    self._start(**kwargs)
  File "/home/vaibhav/mozregression/venv/local/lib/python2.7/site-packages/mozregression/launchers.py", line 126, in _start
    self.runner.start()
  File "/home/vaibhav/mozregression/venv/local/lib/python2.7/site-packages/mozrunner/base/browser.py", line 67, in start
    BaseRunner.start(self, *args, **kwargs)
  File "/home/vaibhav/mozregression/venv/local/lib/python2.7/site-packages/mozrunner/base/runner.py", line 102, in start
    self.process_handler.run(self.timeout, self.output_timeout)
  File "/home/vaibhav/mozregression/venv/local/lib/python2.7/site-packages/mozprocess/processhandler.py", line 669, in run
    self.proc = self.Process([self.cmd] + self.args, **args)
  File "/home/vaibhav/mozregression/venv/local/lib/python2.7/site-packages/mozprocess/processhandler.py", line 97, in __init__
    universal_newlines, startupinfo, creationflags)
  File "/usr/lib/python2.7/subprocess.py", line 710, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1335, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory


Unassigning myself from the bug, as I am busy with other bugs. Will take a look at it again when I find time.

Comment 14

[Julien Pagès (:parkouss)]

(In reply to Vaibhav (:vaibhav1994) from comment #13)
> I tried pulling the latest source code and running it. I am getting this
> error:

Same comment as in Bug 1048858 comment 13.

Comment 15

[Rahul Ranjan]

Hi Julien, I have created a pull request for this issue https://github.com/mozilla/mozregression/pull/192.

Comment 16

[Julien Pagès (:parkouss)]

Hey Rahul, thanks for looking at this!

your PR is not exactly what we want here - in fact the issue has changed over the time, and the title is confusing now (sorry for that). What we need is simply to remove the --inbound flag.

inbound mode will be activated automatically when we will set --good-rev and --bad-rev.

For now, we need to use a command like:

mozregression --inbound --good-rev a7c177546ca0 --bad-rev 986e840a2979

We want just:

mozregression --good-rev a7c177546ca0 --bad-rev 986e840a2979

(inbound mode defined automatically when --good-rev / --bad-rev are set and --inbound is no more an option)


That's it - you will have to adapt some unit tests I presume. Look here to know how to start them: https://wiki.mozilla.org/Auto-tools/Projects/Mozregression

Also after you created your pull request you should ask for a proper review: see http://ateam-bootcamp.readthedocs.org/en/latest/guide/development_process.html#git-and-github.

Last thing I think, we have some code here that will need to be updated also related to this change: https://github.com/mozilla/mozregression/blob/7a32b36ceca758ba95eced2a5898bcf9dec8afe5/mozregression/bisector.py#L425

I mention it because it is easy to not see this. :)

If you have any question, do not hesitate to ask me.

Comment 17

[Rahul Ranjan]

Thank Julien for your feedback. I will make changes accordingly.

Comment 18

[Rahul Ranjan]

I have a question. Are we trying to completely remove the --inbound options or is it for only the case that when --good-rev and --bad-rev is given then we don't need to use it?

Comment 19

[Julien Pagès (:parkouss)]

Yes, we want to completely remove the --inbound option.

Comment 20

[Julien Pagès (:parkouss)]

Hey Rahul,

I just looked at you pull request, it is good! There are just a few things to fix that I put into the comments.

Also, we like to have a clean git history when it's possible. I would like you to squash or fix all your commits into one only. You can do that with a command like:

git rebase -i HEAD~5

(see here for examples and explanations http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html)

Once you have only one commit for the work you have done for this (a commit message like "remove inbound flag" is good to me), you have to push your changes with the force flag since you changed the history:

git push -f

Your PR will automatically be updated, and you will be able to see it on github.

Last thing, once the PR is ready, could you ask for a proper review as stated here:

http://ateam-bootcamp.readthedocs.org/en/latest/guide/development_process.html#git-and-github

You can put me or wlach as the reviewer.

Thank you!

Comment 21

[Rahul Ranjan]

Attachment: remove --inbound flag

Hi Julien, I have stashed the commit into one.

Comment 22

[Julien Pagès (:parkouss)]

Comment on attachment 8571012 [details] [review]
remove --inbound flag

Good stuff, looks all good to me. :)

Comment 23

[Julien Pagès (:parkouss)]

I merged this in. Next release won't have this --inbound flag!

Thank you Rahul. :)

Comment 24

[Rahul Ranjan]

Julien Thanks for the merge :) 

Can you suggest me the next bug I should work on?

Comment 25

[Julien Pagès (:parkouss)]

Hm, well mozregression does not have a lot of bugs now. There are some that already have a owner, and others needs some checks (old bugs not up to date).

You can have a look at bug 1138145, but I'm waiting for the approval of Will for this. it is somewhere related to this bug, and is mainly about removing some code (although there are some refactoring that could be interesting in it).

An other choice if that's interest you is to work on bug 1138153 - it is about adding some unit tests to increase the coverage. I do care about unit tests because that give us confidence when we make some changes and help to ensure that nothing is broken (well there are othe advantages, but I won't list them all here!). It is not easy to write good tests, and often I write more code for test than the actual functionnality - this is an interesting bug if you want to help on this task, and maybe discover python unit testing with the unitest library and mock.