771511 - event.fail() is not propagated correctly for restart tests and mark tests as passed

Status: RESOLVED INVALID

(Testing Graveyard :: Mozmill, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: testcase

This bug is amazing. When an exception is thrown via throw or in any assert.* call the test is not marked as failed if we have a restart test. It works fine for a non-restart test.

In the attached testcase you will get 1 failure for the first normal test and 3 passes for the restart/shutdown tests.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Surprisingly even with Mozmill 1.5 we fail and do not report the test as failed!

TEST-START | /test.js | testUserRestart
TEST-PASS | test.js | test1.js::testUserRestart

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

And for Mozmill 2 we don't even get a final test result if an usershutdown happens:

TEST-START | test/test1.js | testUserRestart
TEST-START | test/test2.js | testRestart

I say wow!

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> And for Mozmill 2 we don't even get a final test result if an usershutdown
> happens:
> 
> TEST-START | test/test1.js | testUserRestart
> TEST-START | test/test2.js | testRestart

That one has now been filed as bug 771517.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So this one is strange and I'm not sure that it can fixed as given by the testcase. Here what I have observed so far:

If you have an assertion/exception in a test and the try/catch statement is used to run the restart logic in 'finally' the application gets restarted *before* we were able to report the failure to the Mozmill framework. We could delay the restart for a bit which probably could fix this issue.

On the other side we could move the restart logic into the teardownTest function and get rid of try/catch. Once the test function has been quit, the failure gets reported correctly to the framework before teardownTest gets executed. But given that this is shared code between different tests we would need a state variable in the persisted object which tell teardownTest what to do. Is it a restart or shutdown, is it an user restart/shutdown, or has the profile to be reset? This makes things more complicated. Additionally we would need a separate teardownModule which handles the final clean-up code. Nearly every time a restart with a profile reset is probably necessary.

So not sure which path we want to follow here and how you think restart tests should work when put together as a single module. We should discuss this in todays meeting.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Opening up for triage given that this only affects restart tests and we didn't wanted to block on such failures. Lets assess the bug again.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This test is not valid anymore. Whenever you are trying to restart the browser from within a module, this task should be done via teardownTest. Never do that inside a test, because if a failure occurs before the restart line, the restart will never happen. 

Please see bug 895657 which will offer us a state machine we can use to execute tests in a defined and dynamic order.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The test on this bug for making sure we catch those failures will be covered by bug 895657.