Add more logic to the restart tests to skip all test files if test1.js failed Me and Henrik had a chat about it on IRC and we wanted to use a flag, something like persisted.testPassed and make a check for it at the beginning of each test file after the first.

This not only belongs to the first test but any test in a restart test folder. So whenever we badly fail with an exception we probably should not continue but only let setupModule/teardownModule run for those files. I wonder if we should do this in our tests/lib modules or in Mozmill itself. The latter option would be very invasive and changes the way how restart tests are handled ATM. So I wouldn't consider this as an option. I would like to see a proposal first before we start with the implementation.

I wonder about those tests that don't really depend on the tests before them. Like MasterPassword where test2.js is skipped (or might as well be unskipped and fail) but we can still check test1.js and test3.js functionality. For dependent tests makes sense to stop since will fail anyway.

Yeah. Valid point. The only way we can control this is to let the test specify that. So having it in the test framework would make it certainly more complex.

(In reply to Andreea Matei [:AndreeaMatei] from comment #2) > I wonder about those tests that don't really depend on the tests before > them. Like MasterPassword where test2.js is skipped (or might as well be > unskipped and fail) but we can still check test1.js and test3.js > functionality. > > For dependent tests makes sense to stop since will fail anyway. I think it's pointless to verify only a subtest out of a testcase of ours. If that is possible, then our test is not thought out right. If not, we don't check for the right thing anyway when checking only a portion of the test. I believe that if any test file fails, skip the rest and mark the whole testcase as failing. But I'm open to more arguments to the contrary.

As Andreea pointed out there are tests out there which need a restart to verify e.g. an element. This test doens't have to be crucial to the tests which will follow later on. A good example are our update tests. Here we always have to run the final test module otherwise final results can not be accumulated.

As we have discussed in the 'ask an expert' session we want to implement it in the following way: * initialize in setupModule of the first test: persisted.restartState = {} * initialize state of the test in setupModule: persisted.restartState.test1 = false * add persisted.restartState.testX = true at the end of the test function (soft assertions will not have any effect) * check for persisted.restartState.testX in setupModule of the next test file and skip the test if we have to * delete persisted.restartState in the teardownModule of the last test file

followed the agreed upon logic

Comment on attachment 661191 [details] [diff] [review] patch v1.0 Review of attachment 661191 [details] [diff] [review]: ----------------------------------------------------------------- This looks like a patch for bug 782918

sorry for that. here is the correct patch.

Comment on attachment 661238 [details] [diff] [review] patch v1.0 Review of attachment 661238 [details] [diff] [review]: ----------------------------------------------------------------- Aside from the comments this is looking good to me. I'd like to get Henrik to review the next patch though for his thoughts. ::: tests/functional/restartTests/testAddons_installTheme/test2.js @@ +10,5 @@ > const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../../data/'); > const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'layout/mozilla.html'; > > function setupModule() { > + if (!persisted.restartState.testInstallTheme) { This isn't being set in testAddons_installTheme/test1.js ::: tests/functional/restartTests/testPreferences_masterPassword/test2.js @@ +9,5 @@ > var utils = require("../../../../lib/utils"); > > > var setupModule = function(module) { > + persisted.restartState.testInvokeMasterPassword = false; This isn't being set in testPreferences_masterPassword/test1.js

addressed Dave's comments.

Comment on attachment 661715 [details] [diff] [review] patch v2.0 Review of attachment 661715 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the update Alex. I was checking the code now and have to say it would have been great if you would have updated only a single test for a WIP, so that we can talk about if the implementation will work. I have some concerns we have to solve now. See my comments below. Beside those please make sure that the final patch will also include the restart tests from the other testruns. ::: tests/functional/restartTests/testAddons_RestartlessExtensionWorksAfterRestart/test2.js @@ +17,5 @@ > + if (!persisted.restartState.testInstallRestartlessExtension) { > + testRestartlessExtensionWorksAfterRestart.__force_skip__ = "testInstallRestartlessExtension has failed"; > + teardownModule.__force_skip__ = "testInstallRestartlessExtension has failed"; > + delete persisted.restartState; > + } Seeing Mozmill 2.0 in-front of us which will even has a kind of state machine I wonder if we really should keep the state of each formerly run test. Looking at the code now I feel we shouldn't do that but simply retain the state of the last ran test. Otherwise it could cause race conditions when we can jump back and forward through tests. @@ +23,4 @@ > controller = mozmill.getBrowserController(); > > // Change pref to show the full url in the location bar > prefs.preferences.setPref(PREF_TRIM_URL, false); I don't think that you even want to continue in setupModule if the former test has been failed.

Alex, you promised to give us an update by Thursday last week. So what's the status here?

(In reply to Henrik Skupin (:whimboo) from comment #12) > Comment on attachment 661715 [details] [diff] [review] > Beside those please make sure that the final patch will also include the > restart tests from the other testruns. The restart tests in the remote testrun have only one test file and do not require this logic. > Seeing Mozmill 2.0 in-front of us which will even has a kind of state > machine I wonder if we really should keep the state of each formerly run > test. Looking at the code now I feel we shouldn't do that but simply retain > the state of the last ran test. Otherwise it could cause race conditions > when we can jump back and forward through tests. I'm thinking we are getting better error messages this way. We debated this in the early stages, but can't remember why we finally went with this implementation. > I don't think that you even want to continue in setupModule if the former > test has been failed. Added an else with that. This patch can be applied on the current repo, so you can run it in a testrun. Do you want an extra patch failing the tests to demo it?

Comment on attachment 664612 [details] [diff] [review] patch v3.0 (In reply to Alex Lakatos[:AlexLakatos] from comment #14) > Created attachment 664612 [details] [diff] [review] > patch v3.0 > > (In reply to Henrik Skupin (:whimboo) from comment #12) > > Comment on attachment 661715 [details] [diff] [review] > > Beside those please make sure that the final patch will also include the > > restart tests from the other testruns. > > The restart tests in the remote testrun have only one test file and do not > require this logic. This patch does not cover the update tests, which are also restart tests. > > Seeing Mozmill 2.0 in-front of us which will even has a kind of state > > machine I wonder if we really should keep the state of each formerly run > > test. Looking at the code now I feel we shouldn't do that but simply retain > > the state of the last ran test. Otherwise it could cause race conditions > > when we can jump back and forward through tests. > > I'm thinking we are getting better error messages this way. We debated this > in the early stages, but can't remember why we finally went with this > implementation. I can see the advantage in the simplicity and flexibility of just retaining the state of the previous test, but also see Alex's point in the improved error messages that we would get. That said, it should be clear from the log which test was previously run and therefore failing.

(In reply to Dave Hunt (:davehunt) from comment #15) > I can see the advantage in the simplicity and flexibility of just retaining > the state of the previous test, but also see Alex's point in the improved > error messages that we would get. That said, it should be clear from the log > which test was previously run and therefore failing. The problem is not the output but the check if the previous test was successful. Think about the following test flow: test1() test2() test3() test2() What will you have to check against in test2? It would have to check all the conditions from possible formerly run test methods. But here you can run into a race condition. What happens if test1() passed but not test3()? Would you skip test2() after test1()? This situation can become way complex if more than two tests are referring to the next test. I'm still against the per test method property.

Alex, do you have an update for us? This bug is waiting for about a week now again.

Comment on attachment 664612 [details] [diff] [review] patch v3.0 I explained my concerns about this handling in last weeks ask an expert meeting and we wanted to get this updated.

(In reply to Henrik Skupin (:whimboo) from comment #17) > Alex, do you have an update for us? This bug is waiting for about a week now > again. I ran into bug 797389 while making the PoC and that made me loose a lot of time debugging. Here is a PoC, I already edited the assert in test1 to fail, but feel free to swicht it back to test for both cases. Also, due to bug 797389, please test with a build older than 09-19-2012 http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-09-19-03-06-02-mozilla-central/

> (In reply to Henrik Skupin (:whimboo) from comment #17) > > Alex, do you have an update for us? This bug is waiting for about a week now > > again. > I ran into bug 797389 while making the PoC and that made me loose a lot of > time debugging. In the future please be not silent but report about an issue like that on the bug. Only that way we can make sure that a streamlined progress can happen.

Comment on attachment 667514 [details] [diff] [review] PoC Review of attachment 667514 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/restartTests/testAddons_changeTheme/test1.js @@ +27,5 @@ > > function setupModule() { > + persisted.restartState = {}; > + persisted.restartState.testName = "testInstallTheme"; > + persisted.restartState.testStatus = false; To me, status doesn't imply a boolean. I would personally prefer something like testPassed or testSuccess.

(In reply to Dave Hunt (:davehunt) from comment #21) > To me, status doesn't imply a boolean. I would personally prefer something > like testPassed or testSuccess. That variable name was in the "Ask an expert" etherpad. Personally, I think testPassed is more readable and would go with that. I wonder, what's Henrik's opinion here?

Comment on attachment 667514 [details] [diff] [review] PoC Review of attachment 667514 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/restartTests/testAddons_changeTheme/test1.js @@ +27,5 @@ > > function setupModule() { > + persisted.restartState = {}; > + persisted.restartState.testName = "testInstallTheme"; > + persisted.restartState.testStatus = false; I agree with Dave here. That's definitely better to understand. Also we probably can remove the 'test' prefix and should change the format to: persisted.restartState = { name: "xyz", passed: true } @@ +73,5 @@ > var plainTheme = addonsManager.getAddons({attribute: "value", > value: persisted.theme[0].id})[0]; > > // Verify that plain-theme is marked to be enabled > + assert.equal(plainTheme.getNode().getAttribute("pending"), "enablew"); Why 'enablew'? Please do not upload broken patches. Everyone can make it invalid on their own.

updated as requested. if this is ok, I'm going to go ahead and update all files.

Comment on attachment 670311 [details] [diff] [review] PoC v2 I think that looks fine now. I would like to also see a review from Dave. When seeing all those changes I wonder if we shouldn't better try to get the exact some behavior right away into Mozmill 1.5. Given that we would also need it for 2.0 it should be a simpler port, as the need to update all of our tests with such a huge amount of code. If we would agree on we could close this bug and file a new one for Mozmill by taking all the investigation and code from here. I would assume that I have to help out with the implementation then.

Comment on attachment 670311 [details] [diff] [review] PoC v2 Review of attachment 670311 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, however given comment 25 we should hold off landing this until a decision has been made over whether this change should be in the tests or Mozmill itself. I believe that's what was in question, please correct me if I'm wrong.

As agreed on in our todays meeting please get the tests updated. Have a special eye on the software update tests which do a final computation for the last teardownModule() call. Also please file a bug for getting it into Mozmill 2.0.

Updated all files. Did not skip teardown in update tests.

Comment on attachment 673873 [details] [diff] [review] patch v4.0 Review of attachment 673873 [details] [diff] [review]: ----------------------------------------------------------------- I've only checked the first couple of restart tests, however the issues I've found will apply throughout. Please update the patch and re-request review. ::: tests/functional/restartTests/testAddons_RestartlessExtensionWorksAfterRestart/test2.js @@ +15,5 @@ > > function setupModule() { > + if (!persisted.restartState.passed) { > + testRestartlessExtensionWorksAfterRestart.__force_skip__ = persisted.restartState.name + " has failed"; > + teardownModule.__force_skip__ = persisted.restartState.name + " has failed"; If we skip teardownModule here we'll potentially never clear PREF_INSTALL_DIALOG or reset the discovery pane URL. ::: tests/functional/restartTests/testAddons_changeTheme/test2.js @@ +19,5 @@ > + tabs.closeAllTabs(controller); > + } > + > + persisted.restartState.name = "testThemeIsInstalled"; > + persisted.restartState.passed = false; You should do this in the same line: persisted.restartState = { name: "testThemeIsInstalled", passed: false }; ::: tests/functional/restartTests/testAddons_changeTheme/test3.js @@ +9,5 @@ > > function setupModule() { > + if (!persisted.restartState.passed) { > + testChangedThemeToDefault.__force_skip__ = persisted.restartState.name + " has failed"; > + teardownModule.__force_skip__ = persisted.restartState.name + " has failed"; By skipping teardownModule here you could be preventing clearing PREF_INSTALL_DIALOG, resetting the discovery pane URL, or deleting persisted.theme. Incidentally, it does not appear that the preference is being cleared.

As discussed, you should not skip teardowns, and ensure that they will not cause a failure if the previous test did not complete. Anything that might cause a failure could be surrounded in a try/catch.

Seems there was no need for try/catches, but if you see a bunch of "if (controller.tabs.length > 1) addonsManager.close();" it's because if there are not enough tabs opened, you'll get a Disconnect Error, even if it's wrapped in a try/catch.

Comment on attachment 679834 [details] [diff] [review] patch v5.0 Review of attachment 679834 [details] [diff] [review]: ----------------------------------------------------------------- Let's wait for bug 801301 to land first.

(In reply to Dave Hunt (:davehunt) from comment #32) > Let's wait for bug 801301 to land first. I assume you meant bug 810301. :)

(In reply to Henrik Skupin (:whimboo) from comment #33) > (In reply to Dave Hunt (:davehunt) from comment #32) > > Let's wait for bug 801301 to land first. > > I assume you meant bug 810301. :) That bug has landed. Removed all occurrences of "if (controller.tabs.length > 1) " from this patch. Can we review it?

Comment on attachment 700484 [details] [diff] [review] patch v6.0 Review of attachment 700484 [details] [diff] [review]: ----------------------------------------------------------------- I made all tests to fail in the first one, so I can see the others being skipped. http://mozmill-crowd.blargon7.com/#/functional/report/b4e97e507d826a04069faf0fc3025e1b As you can see, there are some "controller is not defined" errors, coming from the teardownModule(), that can't see the controller initialized in the else block. ::: tests/functional/restartTests/testAddons_changeTheme/test3.js @@ +24,2 @@ > > + if (controller.tabs.length > 1) addonsManager.close(); Here you still have the controller.tabs.length which fails if the test gets skipped, not being able to find the controller since is not entering in else block. ::: tests/functional/restartTests/testAddons_enableDisableExtension/test4.js @@ +26,3 @@ > > addons.resetDiscoveryPaneURL(); > + if (controller.tabs.length > 1) addonsManager.close(); Same as above. ::: tests/functional/restartTests/testAddons_installFromFTP/test2.js @@ +29,2 @@ > addons.resetDiscoveryPaneURL(); > + if (controller.tabs.length > 1) addonsManager.close(); Same here. ::: tests/functional/restartTests/testAddons_installMultipleExtensions/test2.js @@ +19,4 @@ > } > > function teardownModule() { > + if (controller.tabs.length > 1) addonsManager.close(); Same here. ::: tests/functional/restartTests/testAddons_installTheme/test2.js @@ +27,3 @@ > > addons.resetDiscoveryPaneURL(); > + if (controller.tabs.length > 1) addonsManager.close(); And here. ::: tests/functional/restartTests/testAddons_uninstallTheme/test3.js @@ +24,2 @@ > > + if (controller.tabs.length > 1) addonsManager.close(); Here as well.

Comment on attachment 705366 [details] [diff] [review] patch v6.1 Review of attachment 705366 [details] [diff] [review]: ----------------------------------------------------------------- This is also failing in teardownModules where addonsManager or others are undefined because we declare them in the "else" block of setupModule(), that is not getting run. http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb513a5a2d

Linux: http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd90153a2b1 Windows: http://mozmill-crowd.blargon7.com/#/functional/report/1641bc2f6438595b86015bd90151800e

Comment on attachment 711315 [details] [diff] [review] patch v6.2 Review of attachment 711315 [details] [diff] [review]: ----------------------------------------------------------------- You're missing testInstallUninstallBlocklistedExtension and the update tests.

Added skip logic to Update tests and the to new restart test. Linux testrun_update reports: http://mozmill-crowd.blargon7.com/#/update/report/1641bc2f6438595b86015bd9018f6a5f http://mozmill-crowd.blargon7.com/#/update/report/1641bc2f6438595b86015bd9018f9932

Comment on attachment 711787 [details] [diff] [review] patch v6.3 Review of attachment 711787 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/restartTests/testAddons_installUninstallHardBlocklistedExtension/test4.js @@ +13,5 @@ > function setupModule() { > controller = mozmill.getBrowserController(); > addonsManager = new addons.AddonsManager(controller); > + > + if (!persisted.restartState.passed) { Please fix indentation. @@ +14,5 @@ > controller = mozmill.getBrowserController(); > addonsManager = new addons.AddonsManager(controller); > + > + if (!persisted.restartState.passed) { > + testEnabledAddon.__force_skip__ = persisted.restartState.name + " has failed"; No testEnabledAddon in this test. @@ +27,1 @@ > delete persisted.addon; Indentation issue.

Windows report: http://mozmill-crowd.blargon7.com/#/functional/report/a83c700664548dba07298b74bf37890c Linux report: http://mozmill-crowd.blargon7.com/#/functional/report/a83c700664548dba07298b74bf3805db In both reports testAddons_changeTheme/test2.js is failing but I suspect this is a FF problem as it only happens with today Nightly and also with a clean repo.

(In reply to mario garbi from comment #42) > In both reports testAddons_changeTheme/test2.js is failing but I suspect > this is a FF problem as it only happens with today Nightly and also with a > clean repo. The same test is passing in the CI, so it's more likely to be related to your patch. http://mozmill-ci.blargon7.com/#/functional/report/a83c700664548dba07298b74bf365031

I am reproducing this error with a clean repo without any patches, latest nightly 14/02... checking now why in my case they fail every time.

Please raise this as a new bug so we can investigate it separately.

Windows report : http://mozmill-crowd.blargon7.com/#/functional/report/a83c700664548dba07298b74bf634ca5 Linux report : http://mozmill-crowd.blargon7.com/#/functional/report/a83c700664548dba07298b74bf63be7f The previous fails were due to bug 841384 that we found today.

Comment on attachment 713391 [details] [diff] [review] patch v6.4 Review of attachment 713391 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/restartTests/testAddons_changeTheme/test2.js @@ +54,5 @@ > > controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); > controller.click(restartLink); > + > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); If this is related to the click, then it should be in the same block. Same applies for all files. @@ +60,2 @@ > } > + Please remove this extra line, the repository for default at least was cleared by a contributor and we should try to keep it that way. ::: tests/functional/restartTests/testAddons_installFromFTP/test2.js @@ +49,5 @@ > assert.ok(addonsManager.isAddonInstalled({addon: addon}), > "Extension '" + persisted.addon.id + > "' has been correctly installed"); > +} > + No need for the extra line. ::: tests/functional/restartTests/testAddons_pluginDisabledAfterRestart/test2.js @@ +25,5 @@ > } > > function teardownModule() { > // Enable the plugin that was disabled > + if (persisted.enabledPlugins < 1) { If this is the case, wouldn't teardownModule() be skipped (at line 16)? ::: tests/update/testDirectUpdate/test3.js @@ +21,5 @@ > } > > function teardownModule(module) { > // Prepare persisted object for the next update > persisted.updateIndex++; Shouldn't we need to delete persisted.restartState in the teardowns for update tests as well? ::: tests/update/testFallbackUpdate/test3.js @@ +16,5 @@ > + if (!persisted.restartState.passed) { > + testFallbackUpdate_ErrorPatching.__force_skip__ = persisted.restartState.name + > + " has failed"; > + } else { > + persisted.restartState = {name: "testFallbackUpdate_ErrorPatching", Please add a space after '{' as you do in all files. ::: tests/update/testFallbackUpdate/test4.js @@ +21,5 @@ > } > > function teardownModule(module) { > // Prepare persisted object for the next update > persisted.updateIndex++; Please delete persisted.restartState here as well.

Windows XP report: http://mozmill-crowd.blargon7.com/#/functional/report/f36358d058daf73ddbf781501644d008 Linux report: http://mozmill-crowd.blargon7.com/#/functional/report/f36358d058daf73ddbf78150165e133a

Comment on attachment 715899 [details] [diff] [review] patch v6.5 Review of attachment 715899 [details] [diff] [review]: ----------------------------------------------------------------- Hopefully the final one, before heading to Henrik and Dave :) ::: tests/functional/restartTests/testAddons_installFromFTP/test2.js @@ +48,5 @@ > > assert.ok(addonsManager.isAddonInstalled({addon: addon}), > "Extension '" + persisted.addon.id + > "' has been correctly installed"); > +} You have this going on here: -} \ No newline at end of file +} ::: tests/functional/restartTests/testRestartChangeArchitecture/test5.js @@ +25,5 @@ > function testRestarted64bit() { > expect.equal(Services.appinfo.XPCOMABI, "x86_64-gcc3", > "Successfully restarted in 64bit mode after requesting it"); > + > + persisted.restartState.passed = true; We don't need this since it's the final test and there aren't others to skip.

Comment on attachment 716997 [details] [diff] [review] patch v6.6 Review of attachment 716997 [details] [diff] [review]: ----------------------------------------------------------------- I'm happy with it now, but it needs an update since it's not applying cleanly anymore at testRestartChangeArchitecture. Please then request review from Henrik and Dave to see their thoughts about it.

Updated the patch so it applies cleanly (25.02.2013) and added Henrik and Dave for review.

Comment on attachment 717766 [details] [diff] [review] patch v6.7 Review of attachment 717766 [details] [diff] [review]: ----------------------------------------------------------------- One more thing: ::: tests/functional/restartTests/testAddons_pluginDisabledAfterRestart/test2.js @@ +25,5 @@ > } > > function teardownModule() { > // Enable the plugin that was disabled > addons.enableAddon(persisted.plugin.id); This will fail if in test1.js we fail before setting persisted.plugin, it will be undefined here.

I have an patch message "\ No newline at end of file" at line 531 and after investigating the reason I've found out that a while back we had a patch that was commited with no end of line (this is where this error first appeared). In this patch that problem is fixed even though I still get the message. I've also noticed that default editors do not show the end of line and if there is 1 or 0 (0 lines will bring an error). This test was not touched by our contributor that removed the extra end of lines and this is why only now this has surfaced. The patch that created this: https://bugzilla.mozilla.org/show_bug.cgi?id=709932 Windows reports: http://mozmill-crowd.blargon7.com/#/functional/report/442817abdba0ee856925c7a6010d3c00 Linux reports: http://mozmill-crowd.blargon7.com/#/functional/report/442817abdba0ee856925c7a6010e3497

Comment on attachment 720756 [details] [diff] [review] patch v6.8 Review of attachment 720756 [details] [diff] [review]: ----------------------------------------------------------------- My test report: http://mozmill-crowd.blargon7.com/#/functional/report/00ba6b3792451db672d192b3682c1c89 ::: tests/functional/restartTests/testRestartChangeArchitecture/test5.js @@ +16,5 @@ > + } > +} > + > +function teardownModule() { > + delete persisted.restartState; Sorry I missed this before, but there's no point in running this if we're on other platforms than OS X and all tests are skipped. We should add a line at the last if from this test, skipping teardownModule() as well.

Updated versions including the required modifications.

Comment on attachment 721712 [details] [diff] [review] patch 6.9 Review of attachment 721712 [details] [diff] [review]: ----------------------------------------------------------------- Tests well now. Please ask Henrik and Dave to have a look before we land this.

Comment on attachment 721712 [details] [diff] [review] patch 6.9 Review of attachment 721712 [details] [diff] [review]: ----------------------------------------------------------------- Apologies for the delay in reviewing this, and for the duplicated comments. :) ::: tests/functional/restartTests/testAddons_changeTheme/test1.js @@ +82,5 @@ > parent: plainTheme}); > > controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); > controller.click(restartLink); > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); Why are we adding this assertion here? It doesn't seem relevant to the bug. I see that it was introduced in attachment 711315 [details] [diff] [review] but I can't see a reason in the comments. ::: tests/functional/restartTests/testAddons_changeTheme/test2.js @@ +53,5 @@ > parent: defaultTheme}); > > controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); > controller.click(restartLink); > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); Again, why have we added this assertion? ::: tests/functional/restartTests/testAddons_changeTheme/test3.js @@ +23,5 @@ > } > > function teardownModule() { > + if (persisted.restartState.passed) { > + addons.resetDiscoveryPaneURL(); Why are we only resetting the discovery pane URL and closing the addons manager if the test has passed? This means that a failed test would leave these in a bad state. ::: tests/functional/restartTests/testAddons_enableDisableExtension/test2.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > // Include required modules > +var { assert } = require("../../../../lib/assertions"); Why add this if it's not used? @@ +52,5 @@ > // User initiated restart > controller.startUserShutdown(TIMEOUT_USERSHUTDOWN, true); > > controller.click(restartLink); > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); Why are we adding this assertion? ::: tests/functional/restartTests/testAddons_enableDisableExtension/test3.js @@ +46,5 @@ > // User initiated restart > controller.startUserShutdown(TIMEOUT_USERSHUTDOWN, true); > > controller.click(restartLink); > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); Why are we adding this assertion? ::: tests/functional/restartTests/testAddons_enableDisableExtension/test4.js @@ +25,5 @@ > > function teardownModule() { > + addons.resetDiscoveryPaneURL(); > + > + if (persisted.restartState.passed) { We want to close the addons manager if it's open, and if the test failed the addons manager may still be open... ::: tests/functional/restartTests/testAddons_installFromFTP/test2.js @@ +26,5 @@ > function teardownModule() { > addons.resetDiscoveryPaneURL(); > + > + if (persisted.restartState.passed) { > + addonsManager.close(); As above, we don't just want to close this if the test passes. ::: tests/functional/restartTests/testAddons_installMultipleExtensions/test2.js @@ +23,5 @@ > } > > function teardownModule() { > + if (persisted.restartState.passed) { > + addonsManager.close(); As above, we should attempt to close the addons manager regardless of whether the test passed. ::: tests/functional/restartTests/testAddons_installTheme/test2.js @@ +27,5 @@ > function teardownModule() { > + addons.resetDiscoveryPaneURL(); > + > + if (persisted.restartState.passed) { > + addonsManager.close(); Again, we should still close the addons manager even if the test failed. ::: tests/functional/restartTests/testAddons_installUninstallHardBlocklistedExtension/test4.js @@ +22,5 @@ > } > > function teardownModule() { > + if (persisted.restartState.passed) { > + addonsManager.close(); Again, we should close this regardless of the test outcome. ::: tests/functional/restartTests/testAddons_pluginDisabledAfterRestart/test1.js @@ +24,2 @@ > } else { > testDisablePlugin.__force_skip__= "No enabled plugins detected" We would want to have persisted.restartState.passed be false in this case so the next test is skipped, but instead it will be undefined. We should move setting persisted.restartState to outside of the if statement. ::: tests/functional/restartTests/testAddons_uninstallExtension/test2.js @@ +61,5 @@ > parent: toDisableExtension}); > > controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); > controller.click(restartLink); > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); Why are we adding this assertion? ::: tests/functional/restartTests/testAddons_uninstallExtension/test4.js @@ +45,5 @@ > parent: enabledExtension}); > > controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); > controller.click(restartLink); > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); Why are we adding this assertion? ::: tests/functional/restartTests/testAddons_uninstallExtension/test5.js @@ +23,4 @@ > } > > function teardownModule() { > + if (persisted.restartState.passed) { We should reset this regardless of the outcome of the test. ::: tests/functional/restartTests/testAddons_uninstallTheme/test3.js @@ +25,5 @@ > function teardownModule() { > + addons.resetDiscoveryPaneURL(); > + > + if (persisted.restartState.passed) { > + addonsManager.close(); Again, we should close the addons manager regardless of the test passing. ::: tests/update/testDirectUpdate/test1.js @@ +11,5 @@ > const PREF_UPDATE_LOG = "app.update.log"; > > > function setupModule(module) { > + persisted.restartState = { name: "test1.js::setupModule", passed: false }; I'm not keen on this name, but don't have any better suggestions. ::: tests/update/testFallbackUpdate/test1.js @@ +11,5 @@ > const PREF_UPDATE_LOG = "app.update.log"; > > > function setupModule(module) { > + persisted.restartState = { name: "test1.js::setupModule", passed: false }; As above, this name isn't great, but I still don't have any better suggestions. :)

I left the r? for you intentionally Henrik, but assume you're fine with my review. Perhaps you can take the next round?

That's my intention. I don't want to review an already r- patch again.

(In reply to Dave Hunt (:davehunt) from comment #58) > Comment on attachment 721712 [details] [diff] [review] > patch 6.9 > > Review of attachment 721712 [details] [diff] [review]: > ----------------------------------------------------------------- > > Apologies for the delay in reviewing this, and for the duplicated comments. > :) > > ::: tests/functional/restartTests/testAddons_changeTheme/test1.js > @@ +82,5 @@ > > parent: plainTheme}); > > > > controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); > > controller.click(restartLink); > > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); > > Why are we adding this assertion here? It doesn't seem relevant to the bug. > I see that it was introduced in attachment 711315 [details] [diff] [review] > but I can't see a reason in the comments. While testing the restart logic we've noticed that if the test doesn't click on the restartLink we will get a false positive and fail in the next test with Shutdown expected. This assert makes sure we click the restart link properly. > > ::: tests/functional/restartTests/testAddons_changeTheme/test2.js > @@ +53,5 @@ > > parent: defaultTheme}); > > > > controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); > > controller.click(restartLink); > > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); > > Again, why have we added this assertion? Same as above, I have added this assertion where we had click(restartLink) > > ::: tests/functional/restartTests/testAddons_changeTheme/test3.js > @@ +23,5 @@ > > } > > > > function teardownModule() { > > + if (persisted.restartState.passed) { > > + addons.resetDiscoveryPaneURL(); > > Why are we only resetting the discovery pane URL and closing the addons > manager if the test has passed? This means that a failed test would leave > these in a bad state. > I have moved the resetDiscoveryPaneURL out of the If, the addonsManager.close() should be in this if because if we skip the tests the addon manager never get's opened in the first place. > ::: tests/functional/restartTests/testAddons_enableDisableExtension/test2.js > @@ +2,5 @@ > > * License, v. 2.0. If a copy of the MPL was not distributed with this > > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > > > // Include required modules > > +var { assert } = require("../../../../lib/assertions"); > > Why add this if it's not used? > > @@ +52,5 @@ > > // User initiated restart > > controller.startUserShutdown(TIMEOUT_USERSHUTDOWN, true); > > > > controller.click(restartLink); > > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); > > Why are we adding this assertion? Explained above > > ::: tests/functional/restartTests/testAddons_enableDisableExtension/test3.js > @@ +46,5 @@ > > // User initiated restart > > controller.startUserShutdown(TIMEOUT_USERSHUTDOWN, true); > > > > controller.click(restartLink); > > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); > > Why are we adding this assertion? Explained above > > ::: tests/functional/restartTests/testAddons_enableDisableExtension/test4.js > @@ +25,5 @@ > > > > function teardownModule() { > > + addons.resetDiscoveryPaneURL(); > > + > > + if (persisted.restartState.passed) { > > We want to close the addons manager if it's open, and if the test failed the > addons manager may still be open... AddonManager get's opened in the testEnableAddon, which in turn get's skipped if restartState.passed == false, therefore we won't have an AddonManager to close. > > ::: tests/functional/restartTests/testAddons_installFromFTP/test2.js > @@ +26,5 @@ > > function teardownModule() { > > addons.resetDiscoveryPaneURL(); > > + > > + if (persisted.restartState.passed) { > > + addonsManager.close(); > > As above, we don't just want to close this if the test passes. > > ::: > tests/functional/restartTests/testAddons_installMultipleExtensions/test2.js > @@ +23,5 @@ > > } > > > > function teardownModule() { > > + if (persisted.restartState.passed) { > > + addonsManager.close(); > > As above, we should attempt to close the addons manager regardless of > whether the test passed. > > ::: tests/functional/restartTests/testAddons_installTheme/test2.js > @@ +27,5 @@ > > function teardownModule() { > > + addons.resetDiscoveryPaneURL(); > > + > > + if (persisted.restartState.passed) { > > + addonsManager.close(); > > Again, we should still close the addons manager even if the test failed. > > ::: > tests/functional/restartTests/ > testAddons_installUninstallHardBlocklistedExtension/test4.js > @@ +22,5 @@ > > } > > > > function teardownModule() { > > + if (persisted.restartState.passed) { > > + addonsManager.close(); > > Again, we should close this regardless of the test outcome. > > ::: > tests/functional/restartTests/testAddons_pluginDisabledAfterRestart/test1.js > @@ +24,2 @@ > > } else { > > testDisablePlugin.__force_skip__= "No enabled plugins detected" > > We would want to have persisted.restartState.passed be false in this case so > the next test is skipped, but instead it will be undefined. We should move > setting persisted.restartState to outside of the if statement. > > ::: tests/functional/restartTests/testAddons_uninstallExtension/test2.js > @@ +61,5 @@ > > parent: toDisableExtension}); > > > > controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); > > controller.click(restartLink); > > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); > > Why are we adding this assertion? > > ::: tests/functional/restartTests/testAddons_uninstallExtension/test4.js > @@ +45,5 @@ > > parent: enabledExtension}); > > > > controller.startUserShutdown(TIMEOUT_USER_SHUTDOWN, true); > > controller.click(restartLink); > > + assert.ok(!addonsManager.isOpen, "Addon Manager has been closed"); > > Why are we adding this assertion? > > ::: tests/functional/restartTests/testAddons_uninstallExtension/test5.js > @@ +23,4 @@ > > } > > > > function teardownModule() { > > + if (persisted.restartState.passed) { > > We should reset this regardless of the outcome of the test. > > ::: tests/functional/restartTests/testAddons_uninstallTheme/test3.js > @@ +25,5 @@ > > function teardownModule() { > > + addons.resetDiscoveryPaneURL(); > > + > > + if (persisted.restartState.passed) { > > + addonsManager.close(); > > Again, we should close the addons manager regardless of the test passing. > > ::: tests/update/testDirectUpdate/test1.js > @@ +11,5 @@ > > const PREF_UPDATE_LOG = "app.update.log"; > > > > > > function setupModule(module) { > > + persisted.restartState = { name: "test1.js::setupModule", passed: false }; > > I'm not keen on this name, but don't have any better suggestions. I had a similar problem when naming it, I went with DirectUpdate/test1.js but it looked bad. > > ::: tests/update/testFallbackUpdate/test1.js > @@ +11,5 @@ > > const PREF_UPDATE_LOG = "app.update.log"; > > > > > > function setupModule(module) { > > + persisted.restartState = { name: "test1.js::setupModule", passed: false }; > > As above, this name isn't great, but I still don't have any better > suggestions. :) I will add a patch with the changes in a few moments.

(In reply to mario garbi from comment #61) > > Why are we adding this assertion here? It doesn't seem relevant to the bug. > > I see that it was introduced in attachment 711315 [details] [diff] [review] > > but I can't see a reason in the comments. > > While testing the restart logic we've noticed that if the test doesn't click > on the restartLink we will get a false positive and fail in the next test > with Shutdown expected. This assert makes sure we click the restart link > properly. If we click the restart link Firefox will restart. If Firefox is not open (as expected) when this assertion is executed then I would expect it to fail. Why would the test not click the restart link? This sounds more like a Mozmill bug to me. > > ::: tests/functional/restartTests/testAddons_changeTheme/test3.js > > @@ +23,5 @@ > > > } > > > > > > function teardownModule() { > > > + if (persisted.restartState.passed) { > > > + addons.resetDiscoveryPaneURL(); > > > > Why are we only resetting the discovery pane URL and closing the addons > > manager if the test has passed? This means that a failed test would leave > > these in a bad state. > > > > I have moved the resetDiscoveryPaneURL out of the If, the > addonsManager.close() should be in this if because if we skip the tests the > addon manager never get's opened in the first place. What makes you assume that the addons manager is not open if a test fails? The test could fail after opening the addons manager. > > ::: tests/functional/restartTests/testAddons_enableDisableExtension/test4.js > > @@ +25,5 @@ > > > > > > function teardownModule() { > > > + addons.resetDiscoveryPaneURL(); > > > + > > > + if (persisted.restartState.passed) { > > > > We want to close the addons manager if it's open, and if the test failed the > > addons manager may still be open... > > AddonManager get's opened in the testEnableAddon, which in turn get's > skipped if restartState.passed == false, therefore we won't have an > AddonManager to close. What if testEnableAddon fails _after_ opening the addons manager?

I have added the changes suggested by Dave in the Ask an Expert meeting.

Comment on attachment 731168 [details] [diff] [review] patch v7.0 Review of attachment 731168 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/restartTests/testAddons_changeTheme/test3.js @@ +25,5 @@ > function teardownModule() { > + addons.resetDiscoveryPaneURL(); > + > + if (addonsManager.isOpen) { > + addonsManager.close(); Wouldn't be simpler to change addonsManager.close() method, so it checks if there are any aom tabs to close and if not just skip the closing part? Like we do in private browsing close() method, we wrapped the try-catch part in an if. Dave, Henrik, what do you think?

I would suggest doing that, yes. We might want to do it in another bug, so that we do not pollute those changes into this patch. There are multiple tests to be changed involved here, right?

The current behaviour is to throw an exception if the addons manager is not open. This is expected, and only recently implemented in bug 810301. I would consider this carefully before we change this to silently ignore. Perhaps an additional boolean argument to indicate if we want to throw if the addons manager is not open? Either way, I would do this in another bug.

Comment on attachment 731168 [details] [diff] [review] patch v7.0 Review of attachment 731168 [details] [diff] [review]: ----------------------------------------------------------------- I would like to defer to Henrik for this review.

I will upload the simplified patch after Bug 859241 lands.

Comment on attachment 731168 [details] [diff] [review] patch v7.0 Review of attachment 731168 [details] [diff] [review]: ----------------------------------------------------------------- I will wait with the review then.

Reports for Nightly on 26.04.2013: Linux: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1846d13 http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde18246e6 Mac: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde180cec8 http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde17fece3 WinXP: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde17ff7b8 http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde17f17c1

Comment on attachment 742323 [details] [diff] [review] patch v7.1 Review of attachment 742323 [details] [diff] [review]: ----------------------------------------------------------------- This no longer applies cleanly, also some more changes are needed. ::: tests/functional/restartTests/testAddons_changeTheme/test3.js @@ +23,5 @@ > } > > function teardownModule() { > + addons.resetDiscoveryPaneURL(); > + addonsManager.close(); We should use true argument as we established in the other bug for teardowns, attempt to close it regardless of it's state. This way other changes to the tests won't affect this part of the code. Same applies for all instances in the patch.

Made the changes requested and it applies cleanly now on Default: WinXP: http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e608cc2 http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e609aec Linux: http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e60a6c6 http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e60bb60 Mac OS 10.6.8: http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e60ae7a http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e60c790

Comment on attachment 747822 [details] [diff] [review] patch v7.2 Review of attachment 747822 [details] [diff] [review]: ----------------------------------------------------------------- Looks good now. Henrik, can you please do a final check? Bug 859241 has been landed now.

Updated the patch so it applies cleanly again on default branch.

Comment on attachment 754787 [details] [diff] [review] patch v7.3 Review of attachment 754787 [details] [diff] [review]: ----------------------------------------------------------------- At first I thought there were some indentation problems due to how Bugzilla was showing the long lines, but there aren't. Leaving the final word to Henrik.

Updated the patch to apply correctly and updated the new tests with the restart logic. We had two tests where we had to skip teardown as well: testAddon_enableDisableExtension testAddons_uninstallExtension If the patch looks fine I will provide reports for all platforms. Ubuntu reports: http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219327011 http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219332035

Comment on attachment 809185 [details] [diff] [review] restartLogic_v8.patch Review of attachment 809185 [details] [diff] [review]: ----------------------------------------------------------------- Please clean up the patch, it is already big, let it have only the required changes. ::: tests/functional/restartTests/testAddons_enableDisableExtension/test3.js @@ +5,5 @@ > "use strict"; > > // Include required modules > var addons = require("../../../../lib/addons"); > +var { assert } = require("../../../../lib/assertions"); Given the size on these change, please don't also make stylistic ones. If you want these fixed, file another but for them. ::: tests/functional/restartTests/testAddons_enableDisableExtension/test4.js @@ +29,3 @@ > > addons.resetDiscoveryPaneURL(); > + aModule.addonsManager.close(true); Why are we changing this? Do we have new failures that we want to ignore? ::: tests/functional/restartTests/testAddons_installMultipleExtensions/test1.js @@ +55,5 @@ > /** > * Installs multiple addons > */ > function testInstallMultipleExtensions() { > + persisted.addons.forEach(function (addon) { As the ones mentioned above, please don't change lines just for styling purposes.

Removed the style changes from the patch as requested. Some .close methods need to ignore if the addon manager has been already closed due to test logic, that is why we had the dependency bug that added that parameter to the method. Linux: http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea2b099d http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea2b2d14 Mac: http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea2b195a http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea2b344f Windows: http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea2b5232 http://mozmill-crowd.blargon7.com/#/functional/report/6d6f6a58b02eeffc06eafa8bea2b5f58

Comment on attachment 814038 [details] [diff] [review] restartLogic_v9.patch Review of attachment 814038 [details] [diff] [review]: ----------------------------------------------------------------- There are a few things left: ::: tests/functional/restartTests/testAddons_installUninstallSoftBlocklistedExtension/test3.js @@ +13,5 @@ > aModule.addonsManager = new addons.AddonsManager(aModule.controller); > + > + if (!persisted.restartState.passed) { > + testUninstallBlocklistedExtension.__force_skip__ = persisted.restartState.name + > + " has failed"; This is not intended well. ::: tests/functional/restartTests/testAddons_installUninstallSoftBlocklistedExtension/test4.js @@ +18,5 @@ > aModule.addonsManager = new addons.AddonsManager(aModule.controller); > + > + if (!persisted.restartState.passed) { > + testBlocklistedExtensionUninstalled.__force_skip__ = persisted.restartState.name + > + " has failed"; Same here. ::: tests/functional/restartTests/testAddons_pluginDisabledAfterRestart/test2.js @@ +34,3 @@ > > + delete persisted.plugin; > + } Please add a blank line after the if block. ::: tests/functional/restartTests/testAddons_uninstallTheme/test3.js @@ +19,5 @@ > tabs.closeAllTabs(aModule.controller); > + > + if (!persisted.restartState.passed) { > + testThemeIsUninstalled.__force_skip__ = persisted.restartState.name + > + " has failed"; Wrong indentation. ::: tests/functional/restartTests/testSoftwareUpdateAutoProxy/test2.js @@ +16,5 @@ > testSoftwareUpdateAutoProxy.__force_skip__ = "No permission to update Firefox."; > + > + if (!persisted.restartState.passed) { > + testSoftwareUpdateAutoProxy.__force_skip__ = persisted.restartState.name + > + " has failed"; Same here.

Sorry for the indentation mistakes, I made some quick changes and overlooked them, won't happen again. Functional: http://mozmill-crowd.blargon7.com/#/functional/report/ec449c026814fd64783d738c029f5bd8 Remote: http://mozmill-crowd.blargon7.com/#/remote/report/ec449c026814fd64783d738c029f68bd

Mario will no longer work on this bug. I am setting the bug to new.

We might be able to greatly simply the logic needed here. Since we're now on mozmill 2, we can refactor the restart tests to have a whole restart module in 1 file. This should make the needed changes & logic much smaller here.

Comment on attachment 817215 [details] [diff] [review] v9 Taking the review flags off. I would like to go the route of mentioned in comment 82, to have all related restart tests in a single file.

Oh wow, this is still not fixed? I came here in a moment of melancholy, wasn't expecting this to be still opened :D Hehe, good times.

Mozmill is dead, WONTFIX the remaining bugs.