[exit] mozmill and mozmill-restart should be unified

RESOLVED FIXED

Status

--
enhancement
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: k0scist, Assigned: k0scist)

Tracking

Details

(Whiteboard: [mozmill-2.0+])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
[ported from https://bugzilla.mozilla.org/show_bug.cgi?id=581733#c20]

Mozmill has a very complicated cleanup procedure:

http://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L456

and here

http://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L497

Note that there is no end-of-test stop for MozmillRestart:

http://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L659

What is this actually doing?

 * stop JSBridge (maybe?)
 * stop the runnner
 * clean up the profile

Three things.  So really, it shouldn't be that complicated.  It might require careful work to do this in a robust way, but it's not complicated.  

These blocks of code are really messy.  close_bridge is set to false for
MozmillRestart's call to stop_runner.  This is switched on to determine what
the stop_runner function does.  In the case that it is true, additional cleanup
is done.  Additionally, (fallback) cleanup is done in Mozmill.stop() .

None of this is robust.  Also, none of this is particularly obvious to me as
far as why it is done this way.  I wanted to clean up this control flow a bit,
but I couldn't figure out what was necessary as far as what should be called
and the order it should be called.

this entire control should be reconsidered.  We should make sure that we're
doing what we think we're doing.  We should make this robust. Since it is
vitally important to automation that things are cleaned up correctly, this is
the critical point

We should also figure out what functionality we actually want/need here.  The existing structure has a stop() method that is called on CLI finalization.  If something bad happen, we call it with the fatal argument equals to true.  There is also the stop_runner method.  This is called from stop in Mozmill and following each test in MozmillRestart.  Whether this is the correct granularity is probably evident from an understanding of what we actually want to do (as guided by the code already there, in likelihood).  We might want a cleanup function.  We might want to move some of the logic to JSBridge and Mozrunner.
Blocks: 584470
(Assignee)

Updated

8 years ago
Summary: mozmill needs a robust and unified way of handling restarts → [exit] mozmill needs a robust and unified way of handling restarts
(Assignee)

Updated

8 years ago
Assignee: nobody → jhammel
(Assignee)

Comment 1

8 years ago
So, an entire test run in non-restart mode == one restart test.  Once we have this, we have an advantage wrt nailing down stopping: all code will go through one place.  If something is wrong with e.g. shutdown, we can fix it in one place and in a unified way.

So Mozmill + Mozmillrestart should be merged
Summary: [exit] mozmill needs a robust and unified way of handling restarts → [exit] mozmill and mozmill-restart should be unified
(Assignee)

Comment 2

8 years ago
A unified control flow would look something like this:

* initial setup
* for each test group...
** create a new profile
** for each set of tests
*** start the runner
*** run the tests
*** stop the runner
** cleanup profile
* ensure everything is really stopped
* report results

That look about right?
Sounds fine.
(Assignee)

Comment 4

8 years ago
comment 2 roughly translates to

mozmill = Mozmill(...) # initial setup
for test_group in groups: # this could be a internal method
    mozmill.run(test_group)
mozmill.cleanup() # if necessary; doesn't exist yet

Mozmill.run will look something like this

def run(self, ...):

    self.start()     # create profile, etc
    self.run_tests(...) # run the tests
    self.stop()      # or stop runner; resets the profile

run tests will take, again a number of tests from the group.  A
restart will be performed in between each of the lists of tests

def run_tests(...):

    self.start_runner() # start the runner
    for test in tests:
        # run the test
        frame.runTestFile(test)
    self.stop_runner() # stop the runner

This is supposed to be pseudocode vs code.  Obviously there's a lot
more going on than goes here.

This gives rise to the question:  What is a test group?

A test group is, functionally, a group of a list of tests over which
you want to keep the same profile.  Between test groups, the profile
is reset to the starting state.  So a test group looks something like
this:

# the profile is created before this test group is run
test_group = [ [first_test, second_test, third_test],
               # the runner is restarted here
               [another_set_first_test, another_set_second_test, ...],
               # again the runner is restarted
               ...
             ]
# the profile is reset after this test group
...
(Assignee)

Comment 5

8 years ago
we have a somewhat ambiguous definition of what a restart test
is.  To summarize, from https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L387 :

    "A restart test is a directory that either contains a testPre.js
    and a testPost.js or a directory that contains test1.js, test2.js,
    etc. Subdirectories are not descended into."

    Is this really a definition we're happy with?

testPre.js testPost.js: does anyone use these things anymore?  Can we deprecate? They are not in mozmill-tests
(Assignee)

Comment 6

8 years ago
Another thing worth considering is command line handling.  If you do multiple -ts, is that a test group?  I will go under the working assumption that they are not, as this is the current case, and are just part of an existing test group except for restart tests when it gets more complicated.
(Assignee)

Comment 7

8 years ago
I'm also having a difficult time of how this looks for manifests.  First lets look at the non-manifest case:

 mozmill -t foo -t bar

->
 - collect all tests for 'foo' and 'bar'; run with a singular test group.  For the case where there is foo.js in the foo directory and bar.js in the bar directory, you have tests like (pseudo)

[ [foo.js, bar.js] ] # this is a single test group with a single list of tests

Now, for restart tests, lets say foo and bar are restart test directories

[ [ [foo/test1.js], [foo/test2.js] ],  # first test group
  [ [bar/test1.js], [bar/test2.js] ] ] # second test group

However, how do you note this with manifests?

For the non-restart case, its pretty easy.  While it can be done multiple ways, I'll show a one-file manifest:

[foo/foo.js]
[bar/bar.js]

But how should this look for restart tests?  When do you reset the profile?
(Assignee)

Comment 8

8 years ago
one potential solution to comment 7 is to just note the restart tests by directories:

[DEFAULT]
type = restart

[foo]
[bar]

However, this has the distinct disadvantage that you no longer control the order of the restart tests with manifests (bug 617214).  So we can have one or the other.
(Assignee)

Updated

8 years ago
Depends on: 631945
(Assignee)

Comment 9

8 years ago
Talking to :ctalbert Friday we solidified the loose ends needed to knock out this bug:

 - the python callbacks system will be refactored and no longer tied to the Mozmill class: bug 631945
 - a single profile will be used for the duration of the tests.  this is not a change for Mozmill, but will be a change for restart tests who currently reset the profile every directory: https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L456 . Instead, the tests will be responsible for cleaning up after themselves.  If needed, a `mozmill.resetProfile` event could be provided to make this easy from the JS side.  This should be well-tested to protect against regressions, as we don't currently know if any current behaviour relies on this
 - the test collector functions in MozMill and MozMillRestart will be moved out of the class into globals.  The CLI class will call these functions to add to the test manifests. This will allow a single loop over a single list of lists of tests, the runner started and stopped before each item in the list
(Assignee)

Updated

8 years ago
Blocks: 627794

Comment 10

8 years ago
Comment 9 sounds like the right way to go.  One thing I don't see here is how does our idea of "allowing tests select their next test (after a reboot)" fit into this structure.  i think that would be another JS -> Python event.

Something like:
* initial setup
* create profile
* for each test group
*** start the runner
*** run the tests (allowing your test to pick it's follower (optionally).  If not picked, then the next test from the manifest or the next thing in alpha order from a directory is run.
*** (optionally) reset the profile to "starting state"
*** stop the runner
* ensure everything is really stopped
* report results

I think that a test group is either a directory (when we are walking directory trees to find tests) or it is a type of grouping in the manifest.  I think perhaps when you're using manifests you can specify an attribute on the tests that they belong to a test group.  If the test does not belong to a test group, then it simply falls into the "general" test list bucket with all the other tests.  Does that sound like a workable solution?
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]
(In reply to comment #9)
>  - a single profile will be used for the duration of the tests.  this is not a
> change for Mozmill, but will be a change for restart tests who currently reset
> the profile every directory:
> https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L456

Please, please don't do that per default. We have a couple of areas which can't be reset that easily. Think about the persisted properties for XUL elements, which are stored in localstore.rdf. Also tests will fail or not being able to implement in the future, which absolutely require a fresh and clean profile. Killing that you would also kill a special feature we have with Mozmill.
Version: Trunk → unspecified
(Assignee)

Comment 12

8 years ago
(In reply to comment #11)
> (In reply to comment #9)
> >  - a single profile will be used for the duration of the tests.  this is not a
> > change for Mozmill, but will be a change for restart tests who currently reset
> > the profile every directory:
> > https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L456
> 
> Please, please don't do that per default. We have a couple of areas which can't
> be reset that easily. Think about the persisted properties for XUL elements,
> which are stored in localstore.rdf. Also tests will fail or not being able to
> implement in the future, which absolutely require a fresh and clean profile.
> Killing that you would also kill a special feature we have with Mozmill.

Not sure if I understand or if you are giving an alternate proposal.  Currently, the profile is reset in between directories of MozMill restart tests.  Also as best I can tell, this is not documented.  Do you want the profile reset more often?  Less often?  Is there any reason that providing an event to reset the profile won't suffice? What tests does this currently impact?

Comment 13

8 years ago
(In reply to comment #12)
> (In reply to comment #11)

> Not sure if I understand or if you are giving an alternate proposal. 
> Currently, the profile is reset in between directories of MozMill restart
> tests.  Also as best I can tell, this is not documented.  Do you want the
> profile reset more often?  Less often?  Is there any reason that providing an
> event to reset the profile won't suffice? What tests does this currently
> impact?
I think that he's saying when you run a group of tests currently (aka a directory, for instance) that you run using the same profile for that entire run.  This is a good thing, I agree with Henrik. I think that he doesn't realize that we're not advocating changing that, we're just making it run this way by default with both "normal" and "restart" tests.  However, I think that having an event to "reset the profile to the startup state" would be useful for test authors.

We are not advocating having the framework reset the test state on any kind of regular basis.  We would only do that when the tests are started, and when the test has asked us to.  So if the tests do not use this api, then nothing changes (in their world).
(Assignee)

Comment 14

8 years ago
Yes, exactly.  The profile will not get reset, by default.  It will only be reset when its told to via a test.  From a test author's point of view this is all that changes:  the profile gets reset less often (for restart tests) unless you tell it otherwise.  You could have a special test listed in your manifest that just reset the profile, if that became in high demand.
(Assignee)

Comment 15

8 years ago
Other things worth thinking about for the future:

- when running restart tests not from manifests, you do `mozmill --restart -t ...` and all tests added with `-t` are restart tests.  It might make more sense to use different flags (-t for normal tests, -r for restart tests) so that the two can live on the same command line in perfect harmony

- again when running restart tests, we gather tests in a different way, namely looking for tests like `test1.js`, `test2.js` ... ; is there any reason not to just do all the `test*js` tests in the same directory?

I'll preserve the existing functionality for now, though I think its important to think in terms of "what do we really want to do?" vs. "this is what we do now"
(In reply to comment #15)
> Other things worth thinking about for the future:
> 
> - when running restart tests not from manifests, you do `mozmill --restart -t
> ...` and all tests added with `-t` are restart tests.  It might make more sense
> to use different flags (-t for normal tests, -r for restart tests) so that the
> two can live on the same command line in perfect harmony

I like this idea, it would only allow us to do more than we can now.

> - again when running restart tests, we gather tests in a different way, namely
> looking for tests like `test1.js`, `test2.js` ... ; is there any reason not to
> just do all the `test*js` tests in the same directory?

It seems like it would be better to specify the ordering via the command line '-r test1.js,test2.js,test3.js' or via the manifest file. Less file naming magic (and more descriptive file names) and more flexibility.
(Assignee)

Comment 17

8 years ago
(In reply to comment #16)
> (In reply to comment #15)
> > Other things worth thinking about for the future:
> > 
> > - when running restart tests not from manifests, you do `mozmill --restart -t
> > ...` and all tests added with `-t` are restart tests.  It might make more sense
> > to use different flags (-t for normal tests, -r for restart tests) so that the
> > two can live on the same command line in perfect harmony
> 
> I like this idea, it would only allow us to do more than we can now.

I'll endeavor to remember to ticket :)

> > - again when running restart tests, we gather tests in a different way, namely
> > looking for tests like `test1.js`, `test2.js` ... ; is there any reason not to
> > just do all the `test*js` tests in the same directory?
> 
> It seems like it would be better to specify the ordering via the command line
> '-r test1.js,test2.js,test3.js' or via the manifest file. Less file naming
> magic (and more descriptive file names) and more flexibility.

Yeah, if you specify by file they should be ordered thusly.  The question is, if you're running a directory should the test order be the same as for non-restart tests?  I would maintain yes, but again will ticket this as a separate issue after this refactor.  I'll preserve functionality for now.
(In reply to comment #12)
> Currently, the profile is reset in between directories of MozMill restart
> tests.  Also as best I can tell, this is not documented.  Do you want the
> profile reset more often?  Less often?  Is there any reason that providing an
> event to reset the profile won't suffice? What tests does this currently
> impact?

Sorry when I misread your statement. We have to make sure that the profile has to be reset between different directories. That's all. Resetting the profile in between tests inside the same directory would be great. It's something we would need for our l10n tests. If we can get this additional option, I would be absolutely in favor for it!

(In reply to comment #15)
> - again when running restart tests, we gather tests in a different way, namely
> looking for tests like `test1.js`, `test2.js` ... ; is there any reason not to
> just do all the `test*js` tests in the same directory?

If we go with -t vs. -r and we specify a folder, I would like to see the test files renamed! Right now the name doesn't say anything to us and any tiny bit has to be part of the included test function name. Would it be possible to break the naming schema and use i.e. 01_testDownloadUpdate.js instead? That's really something which wasn't able to do with the old collector, but what about now? That would be a big help!

(In reply to comment #16)
> It seems like it would be better to specify the ordering via the command line
> '-r test1.js,test2.js,test3.js' or via the manifest file. Less file naming
> magic (and more descriptive file names) and more flexibility.

The downside of that would be that you will have to specify each single test in that folder. It's impossible when you want to run against a directory which contains a list of directories with restart tests. I don't think that way is doable. But as stated above, I'm in favor of a better naming schema.
(Assignee)

Comment 19

8 years ago
Another thing I'm running up against here is that we wait and kill the runner in two different ways albeit with the same intent for mozmill and mozmill --restart (with ignoring the closing of the bridge and the backchannel):

https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L314

I'm not sure why we do this two different ways or even if it makes a difference.  I'll try them both and hope to decipher what we actually want to do.  Probably this functionality should be moved to the runner anyway:  wait(30) should wait for the program for 30s and return True or False depending on whether the action succeeds or not
(Assignee)

Comment 20

8 years ago
Created attachment 510855 [details] [diff] [review]
preliminary implementation

Preliminary merge.  Its pretty good and about 90% there and can be pushed as is, if desired, and other issues reticketed.  Things I want to do but aren't done:

* make a -r vs a -t flag for restart tests; this way, both can be specified on the command line.
* add a resetProfile event....i'm a bit confused what we actually want to do here
* fix the collect things to work better with manifests, though i might punt this to another bug for the sake of calling this done

As best as my testing tells me, this works.  There may be minor regressions wrt stopping conditions, but at least we can fix them sensibly now.  Overall I find it much easier to read and it removes several previous limitations about how mozmill works (i.e. being one shot....now it is not)

The patch is a bit hard to read.  You can see the whole file at https://github.com/k0s/mozmill/blob/bug-584464/mozmill/mozmill/__init__.py
Attachment #510855 - Flags: feedback?(ctalbert)
(Assignee)

Comment 21

8 years ago
One thing to note re -r and -t, and -m for that matter...

We *can* use -r and -t to let you specify which restart + normal tests to run.  The change will not be hard, either as part of this bug or subsequently;  but with OptionParser, we can't descriminate which test order between the -r and -t options we want to run.  The restart tests will be in order.  The non-restart tests will be in order.  And the manifest tests will be in order.  However, they will not be intermingled in order.

If no one finds this objectionable, I can make this easy change

Comment 22

8 years ago
(In reply to comment #18)
> (In reply to comment #12)
> (In reply to comment #15)
> > - again when running restart tests, we gather tests in a different way, namely
> > looking for tests like `test1.js`, `test2.js` ... ; is there any reason not to
> > just do all the `test*js` tests in the same directory?
> 
> If we go with -t vs. -r and we specify a folder, I would like to see the test
> files renamed! Right now the name doesn't say anything to us and any tiny bit
> has to be part of the included test function name. Would it be possible to
> break the naming schema and use i.e. 01_testDownloadUpdate.js instead? That's
> really something which wasn't able to do with the old collector, but what about
> now? That would be a big help!
If you want to really name tests and not have them be in directory order, you should use a manifest.  That's what it gets you.  The reason they had to be test1, test2 and so on is because the old style method simply read things in directory order. If you use the "-t/-r <directory>" option, then they will still be read in directory order.
> 
> (In reply to comment #16)
> > It seems like it would be better to specify the ordering via the command line
> > '-r test1.js,test2.js,test3.js' or via the manifest file. Less file naming
> > magic (and more descriptive file names) and more flexibility.
Use a manifest.  Honestly, it takes you just as much time to write that command line as it does to write the manifest and call it with -m.

We are keeping the -t/-r switches for *DEBUGGING ONLY*, we aren't going to fix bugs in the ordering in those switches, they'll remain the way they are right now.

We really want to encourage everyone to start using manifests when you're doing anything serious.  If you're just doing a quick check on something, then yes, use -t/-r.  But for real testing, use -m.  The order will be defined, the tests can be descriptively named, and we can get away from having a lot of complicated code that is prone to errors and hard to maintain.

Also, for the above reason I'm also against allowing you to specify both -t and -m or -t, -r, -m all on the same command line.  I think when you invoke mozmill once, you can specify one of -m|-t|-r and those are the tests that are run in that instance of mozmill.  This keeps things simple.  There really is no end to the rabbit hole if we start mixing these and all the answers are right answers therefore I'm really against it.  Better to keep it simple, keep it explicit.

Comment 23

8 years ago
(In reply to comment #20)
> Created attachment 510855 [details] [diff] [review]
> preliminary implementation
> 
> Preliminary merge.  Its pretty good and about 90% there and can be pushed as
> is, if desired, and other issues reticketed.  Things I want to do but aren't
> done:
> 
I'm loving the patch, it looks really good.

> * make a -r vs a -t flag for restart tests; this way, both can be specified on
> the command line.
why not just a -t with --restart?

> * add a resetProfile event....i'm a bit confused what we actually want to do
> here
This is hard because we can't do it when the application is running unless we were somehow able to listen to all the changes that tests make and revert them.

Hmmm....
So, one option, almost entirely JS:
* Option 1: listen to all the changes tests make to their environment and reset them (or listen only to preference changes and reset those and call it good enough.  This would be entirely in JS, and is probably not too hard.  If a test intalls an extension or something we won't be reverting that though.  So this would more aptly be called "revertPreferences" which is probably what tests want 90% of the time. (and was what I was thinking of when I proposed this hairbrained idea).

* Option 2: 
* Add a resetProfile into frame.js that the tests can call.
* Ensure this calls back to a function on the python side
* The python side sets a flag to "reset the profile" back to the original state that mozrunner first created it in
* We check that flag *after the test completes* - it's really the only way to do it
* If flag set, we kill the app, reset the profile, start the app and we pick up with the next test in our sequence.
This option has the benefit of being much more robust and truly resetting the entire profile.  The only issue here is that a test can only use this at the end of its actions.  So the API would be something like "resetProfileAfterCompletion()".

Option 2 sounds good, it would basically allow test authors to write the following test with great abandon:
resetProfileAfterCompletion();
doCrappyThingsToProfile();
doMoreCrappythingsToProfile();
...

I'm happy with punting this piece to a follow on bug.

> * fix the collect things to work better with manifests, though i might punt
> this to another bug for the sake of calling this done
I think this should be done on this patch now.  That's the one of the critical pieces of this refactoring and so I think it should be done before this lands.

You're nearly there.  We can punt on the idea of collections and queues and just ensure that you have a solid bit of code that's easily maintainable for collecting all the bits when we're running.

We can attack the collections/queue idea in another bug if we think they have any merit. 

The patch is looking good, ensure there are no XXX comments, ensure the possible inifinite loop is either timed out or completely eliminated, ensure that we don't ever walk off the end of an except silently unless we really really mean to (like in that bridge disconnect case in stop_runner)

Almost there, going to mark f+

Updated

8 years ago
Attachment #510855 - Flags: feedback?(ctalbert) → feedback+
(Assignee)

Comment 24

8 years ago
See comments inline.

(In reply to comment #23)
> (In reply to comment #20)
> > Created attachment 510855 [details] [diff] [review]
> > preliminary implementation
> > 
> > Preliminary merge.  Its pretty good and about 90% there and can be pushed as
> > is, if desired, and other issues reticketed.  Things I want to do but aren't
> > done:
> > 
> I'm loving the patch, it looks really good.
> 
> > * make a -r vs a -t flag for restart tests; this way, both can be specified on
> > the command line.
> why not just a -t with --restart?

Since we're not allowing mixing restart tests and non-restart tests specified by the command line, yes.  -r was just a proposal to allow that to be done.

> > * add a resetProfile event....i'm a bit confused what we actually want to do
> > here
> This is hard because we can't do it when the application is running unless we
> were somehow able to listen to all the changes that tests make and revert them.
> 
> Hmmm....
> So, one option, almost entirely JS:
> * Option 1: listen to all the changes tests make to their environment and reset
> them (or listen only to preference changes and reset those and call it good
> enough.  This would be entirely in JS, and is probably not too hard.  If a test
> intalls an extension or something we won't be reverting that though.  So this
> would more aptly be called "revertPreferences" which is probably what tests
> want 90% of the time. (and was what I was thinking of when I proposed this
> hairbrained idea).
> 
> * Option 2: 
> * Add a resetProfile into frame.js that the tests can call.
> * Ensure this calls back to a function on the python side
> * The python side sets a flag to "reset the profile" back to the original state
> that mozrunner first created it in
> * We check that flag *after the test completes* - it's really the only way to
> do it
> * If flag set, we kill the app, reset the profile, start the app and we pick up
> with the next test in our sequence.
> This option has the benefit of being much more robust and truly resetting the
> entire profile.  The only issue here is that a test can only use this at the
> end of its actions.  So the API would be something like
> "resetProfileAfterCompletion()".

> Option 2 sounds good, it would basically allow test authors to write the
> following test with great abandon:
> resetProfileAfterCompletion();
> doCrappyThingsToProfile();
> doMoreCrappythingsToProfile();
> ...
> 
> I'm happy with punting this piece to a follow on bug.

Sounds good.  Sorry, I was thinking Option 2.  I should have spelled it out a bit more.  Though actually I like Option 1 for the cases where it applies.  Ideally, a test would be an object (don't run away!) with a setPreference function.  Then the cleanup for the object would automagically revert whatever prefs were set by the test.  When applicable, Option 1 avoids a restart, which I tend to be in favor of when we can avoid it.
 
> > * fix the collect things to work better with manifests, though i might punt
> > this to another bug for the sake of calling this done
> I think this should be done on this patch now.  That's the one of the critical
> pieces of this refactoring and so I think it should be done before this lands.
> 
> You're nearly there.  We can punt on the idea of collections and queues and
> just ensure that you have a solid bit of code that's easily maintainable for
> collecting all the bits when we're running.

> We can attack the collections/queue idea in another bug if we think they have
> any merit. 

Sounds good.  A e.g. queue will be necessary if you want to have the JS specify the next test.  However, we might want to rethink this a bit.  Will follow up later.

> The patch is looking good, ensure there are no XXX comments, ensure the
> possible inifinite loop is either timed out or completely eliminated, ensure
> that we don't ever walk off the end of an except silently unless we really
> really mean to (like in that bridge disconnect case in stop_runner)
> 
> Almost there, going to mark f+

I got most of them.  It turns out some variation on https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L410 is necessary.  I'll play with it a bit more and add some timeout there.  Note that this will never actually loop forever though, since you'll get a JSBridgeDisconnect before that
(Assignee)

Comment 25

8 years ago
Created attachment 511777 [details] [diff] [review]
combine MozMill and MozMillRestart classes, deprecate the latter, and simplify workflow

This looks about what needs to be done for this particular bug.  Other issues should be reticketed.  I've tested as best I can but of course edge cases are difficult to tests (and our existing tests are kinda broken), so it is possible that there are a few edge case regressions (Its also possible that there aren't; again, hard to tell).  The good news, however, is that since the structure is simpler and more malleable, they should be easier to fix.

Another thing somewhat accidentally done by this patch is that MozMill is no longer a one-shot -- you can keep running groups of tests tell the cows come home.
Attachment #510855 - Attachment is obsolete: true
Attachment #511777 - Flags: review?(ctalbert)
(Assignee)

Updated

8 years ago
Blocks: 627160
(Assignee)

Updated

8 years ago
Blocks: 632236
(Assignee)

Updated

8 years ago
Blocks: 568998
(Assignee)

Updated

8 years ago
Blocks: 631288

Updated

8 years ago
Duplicate of this bug: 521396

Updated

8 years ago
Blocks: 604367

Comment 27

8 years ago
Comment on attachment 511777 [details] [diff] [review]
combine MozMill and MozMillRestart classes, deprecate the latter, and simplify workflow

This looks good, thanks Jeff
Attachment #511777 - Flags: review?(ctalbert) → review+
(Assignee)

Comment 28

8 years ago
pushed to master https://github.com/mozautomation/mozmill/commit/f54c70ffc7bedfb3b3ba0bc8f5c8960ad6594cf7
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Duplicate of this bug: 510960
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.