Closed Bug 895657 Opened 7 years ago Closed 7 years ago

State machines will not work because setupTest() and teardownTest() are only called once at the beginning and end of a module

Categories

(Testing Graveyard :: Mozmill, defect, major)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

Details

(Whiteboard: [mozmill-2.0+][ateamtrack: p=mozmill q=2013q3 m=1])

Attachments

(2 files, 2 obsolete files)

If you have a test like the one which is attached you would assume to have the following order of tests to be executed:

*** setupModule ***

*** setupTest ***
*** testFirst ***
*** teardownTest ***

*** setupTest ***
*** testThird ***
*** teardownTest ***

*** setupTest ***
*** testSecond ***
*** teardownTest ***

But what we have right now is this:

*** setupModule ***

*** setupTest ***
*** testFirst ***
*** testSecond ***
*** testThird ***
*** teardownTest ***

This will not allow us to implement any kind of state machine for restart tests. This was our original intention, which we raised during one of our workweeks in London last year.

So this should really block the Mozmill 2.0 release.
And I recall that we looked into the idea, decided it was too costly and complicated to create and decided to ditch it.

You can still do something like it by hand:

setupTest
testFirst
setuptest2 -- called explicitly from testFirst
testTwo
teardownTest2 -- called from a finally block in testTwo
teardowntest

So, I really don't see the need for such a thing. Especially since we have never had a need for a test like this in any of the restart tests to date.  Let's keep our priorities on getting the minimal system out the door and not trying to do everything. If this is really necessary then we can do it later.  I don't see any clear need for this feature.
No, there are needs which we cannot fulfill since ages. The first time the request came up was with bug 604367, which then has been marked as WFM by yourself more than 2 years ago. And I never really verified that since yesterday. As stated here in comment 0 this feature does not work. When restarting an application we can only select a test inside a module to execute next, but not another module. And with incorrect handling of setupTest and teardownTest we are coming not closer. Putting all the restart logic into the tests should not be the way to go, given that any previous failure will drop that request. This also includes your proposal above by explicitly calling a setuptest2 method from the first test. Main reason that this will not work is that possible exceptions are NOT been reported and tests which fail are reported as pass! Please see bug 771511. This is really an unacceptable situation, which we cannot workaround that easily.

Also I already have a working solution on my disk, done within 30 minutes. Existing tests are not affected as far as I can see, and run perfectly. So far I do not see a regression. So I have to conclude that it can't be that costly and hard to get implemented. With all of my latest work I got such a good inside into frame.js that I'm really sure to be able to solve this in a good manner.

If you still disagree, we would have to completely drop the idea of having multiple test methods in a module, and should raise failures if a module is doing that. Further the restartApplication logic would have to be changed, so we can jump to specific test modules. Such a change would involve the Python side too, so I would call it more complex.
Flags: needinfo?(ctalbert)
Attached patch WIP v1 (obsolete) — Splinter Review
This WIP just demonstrates what's necessary here. It's really only a movement of the appropriate methods into the loop where we run our tests. That's all what I can see as of now.
Ok, so perhaps this has changed since I last looked at this code. I think that the work you've done over the last two weeks have made this far eaiser. Let's try to go ahead and get this in given how simple the patch is.
Flags: needinfo?(ctalbert)
Attached patch WIP v2 (obsolete) — Splinter Review
This is most likely the final refactoring necessary to allow a state machine for restart tests. Dave let me know what you think. I will add tests by tomorrow.

Andrei, please run a patched version of Mozmill with our current mutt tests and mozmill-tests. Do you see any strange behavior?
Attachment #778359 - Attachment is obsolete: true
Attachment #779493 - Flags: feedback?(dave.hunt)
Attachment #779493 - Flags: feedback?(andrei.eftimie)
Comment on attachment 779493 [details] [diff] [review]
WIP v2

Review of attachment 779493 [details] [diff] [review]:
-----------------------------------------------------------------

Have not encountered any problems on mutt and mozmill-tests
Looks good.
Attachment #779493 - Flags: feedback?(andrei.eftimie) → feedback+
Attached patch Patch v1Splinter Review
This is hopefully the final patch. A couple of new tests have been added, and the end_test one removed given that is covered by the new ones. All works fine on Linux, but I haven't tested it yet on OS X and Windows. That will follow in a bit.
Attachment #779493 - Attachment is obsolete: true
Attachment #779493 - Flags: feedback?(dave.hunt)
Attachment #779705 - Flags: review?(dave.hunt)
Ran mutt tests on Linux, OS X, and Windows 7. All runs fine without any issue visible.
Comment on attachment 779705 [details] [diff] [review]
Patch v1

Review of attachment 779705 [details] [diff] [review]:
-----------------------------------------------------------------

This is a really nice refactoring job, Henrik! I had a couple of questions, but r+ from me with or without these addressed.

::: mutt/mutt/tests/python/js-modules/restartTests/testStateMachine.js
@@ +10,5 @@
> +  "testThird"
> +];
> +
> +
> +function setupModule(aModule) {

Do we also have a teardownModule at all?

::: mutt/mutt/tests/python/test_restart.py
@@ +15,5 @@
> +                passes=0, fails=0, skips=0):
> +
> +        abspath = os.path.dirname(os.path.abspath(__file__))
> +
> +        if manifest_path:

We're only using test paths, so do we need this logic?
Attachment #779705 - Flags: review?(dave.hunt) → review+
(In reply to Dave Hunt (:davehunt) from comment #9)
> > +function setupModule(aModule) {
> 
> Do we also have a teardownModule at all?

We have but if you have all the logic in teardownTest it will not be executed due to stopApplication(). But let me see that I can really move this into teardownModule(), just for this test.

> ::: mutt/mutt/tests/python/test_restart.py
> @@ +15,5 @@
> > +                passes=0, fails=0, skips=0):
> > +
> > +        abspath = os.path.dirname(os.path.abspath(__file__))
> > +
> > +        if manifest_path:
> 
> We're only using test paths, so do we need this logic?

Oh, I had a manifest before in there. But it's not used anymore, correct. I would prefer to keep that logic so we could create our own test class in the future. That would be a good template then.
(In reply to Henrik Skupin (:whimboo) from comment #10)
> > Do we also have a teardownModule at all?
> 
> We have but if you have all the logic in teardownTest it will not be
> executed due to stopApplication(). But let me see that I can really move
> this into teardownModule(), just for this test.

After some observations we cannot do that. Reason is that all the state machine logic has to be part of teardownTest. That also includes the restartApplication and stopApplication calls. If we would move the latter to teardownModule, Mozmill would continue to execute other tests in that module. That would break the state machine.

To allow something like that and to make it easier to create state machines, we might think of an API for mozmill 2.1. But that's clearly not in scope for 2.0. So lets test this first.
Landed as:
https://github.com/mozilla/mozmill/commit/d8bffb97eee0a002562f2b2f77548869e77e4bc7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+][ateamtrack: p=mozmill q=2013q3 m=1]
Flags: in-testsuite+
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.