767344 - Move --restart flag into Mozmill base class

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Right now it's only possible to run isolated Mozmill tests with restarts between each test module via the Mozmill CLI class. This feature we really need for Mozmill base.

This is really something we wanted to have from Mozmill 2 and given that it's not possible for our automation scripts to easily set this mode, I would request it for Mozmill 2.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Attachment: Patch

Pointer to Github pull-request

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Comment on attachment 636701 [details]
Patch

Jeff, I wonder if we could get rid of the MozMill.run_tests() method and put all its code directly into run(). Or is there any reason why we should keep it?

Comment 3

[cmtalbert]

Comment on attachment 636701 [details]
Patch

Jeff, I need you to look at this one. Henrik is moving some of the disconnect exceptions around, and I know all that code is rather finnicky w.r.t. Mozmill stopping.

Henrik if you want to just get this knocked off your list, I'd give you an r+ to this patch with the restart changes and without the exception changes (do the exception stuff in a follow-on bug).

But with both the exception changes and the restart changes in the patch, I'm going to defer to Jeff for review.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

I will wait for Jeff's comments on it and I have made a couple of comments on the pull request. Until this got r+ I will work on other mozmill-2.0 blockers.

Comment 5

[Jeff Hammel]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Comment on attachment 636701 [details]
> Patch
> 
> Jeff, I wonder if we could get rid of the MozMill.run_tests() method and put
> all its code directly into run(). Or is there any reason why we should keep
> it?

I can't think of a particular reason against combining run_tests and run.  They are mostly separate for improved readability, as the exception handling will result in a further level of indentation if the methods are combined

Comment 6

[Jeff Hammel]

Comment on attachment 636701 [details]
Patch

Instead of having restart passed to run and run_tests and the restarting logic being done in run_tests I am more inclined to have run_tests function as it does currently and have run have the restarting logic.  TBH, to me this is something that should be done from the caller, but having it in run is fine

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Comment on attachment 636701 [details]
Patch

As discussed on IRC we have removed the run_tests() method and moved all its code directly into run().

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Pushed as:
https://github.com/mozautomation/mozmill/commit/5332039e7db1d49b5a11037e865e5abea9f1fd3d

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Oh, boy. No-one forced me to update the Mutt tests. Nearly all of them were broken because of this change. I have pushed an update to get those fixed:

https://github.com/mozautomation/mozmill/commit/8ca929627a5b00f0387275291477984bc85a47a3