File: firefox/testFormManager/testClearFormHistory.js This module consists of two tests: * testSaveFormInformation() * testClearFormHistory() We should split these into two separate modules. I recommend creation of a testSaveFormInformation.js module.

Move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

Alex Lakatos changed story state to started in Pivotal Tracker

* testSaveFormInformation.js - https://litmus.mozilla.org/show_test.cgi?id=15518 * testClearFormHistory.js - https://litmus.mozilla.org/show_test.cgi?id=15517

Comment on attachment 531907 [details] [diff] [review] patch v1.0 >+++ b/tests/functional/testFormManager/testClearFormHistory.js > * The Initial Developer of the Original Code is Mozilla Corporation. > * Portions created by the Initial Developer are Copyright (C) 2009 Please update the year to 2011. >+var testClearFormHistory = function() { Please use the format "function testFunction() {" >+// testClearFormHistory.meta = {litmusids : [15517]}; I don't think we use this anymore -- please remove it. >+++ b/tests/functional/testFormManager/testSaveFormInformation.js >+var testSaveFormInformation = function() { Please make this a named function. >+ // TODO: Restore after 1.5.1 lands >+ // controller.waitFor(function() { >+ // return popDownAutoCompList.getNode().popupOpen == true; >+ // }, TIMEOUT, 100, "Autocomplete popup is open"); Henrik, can this be restored since we are on Mozmill 1.5.3? >+ // TODO: Restore after 1.5.1 lands >+ // controller.waitFor(function() { >+ // return popDownAutoCompList.getNode().popupOpen == true; >+ // }, TIMEOUT, 100, "Autocomplete popup is open"); >+ Henrik, same here... >+ controller.waitForEval("subject.popupOpen", TIMEOUT, 100, >+ popDownAutoCompList.getNode()); Can you change this to a waitFor() so we can get a better error message? See other tests for an example. >+/** >+ * Map test functions to litmus tests >+ */ >+// testSaveFormInformation.meta = {litmusids : [15518]}; I think it's safe to remove this.

Comment on attachment 532896 [details] [diff] [review] patch v2.0 >+ controller.waitFor(function() { >+ return popDownAutoCompList.getNode().popupOpen == true; >+ }, TIMEOUT, 100, "Autocomplete popup is open"); >+ controller.waitFor(function() { >+ return popDownAutoCompList.getNode().popupOpen == true; >+ }, TIMEOUT, 100, "Autocomplete popup is open"); Can you change both of these waitFor() to use the following: * === instead of == * "Autocomplete popup is open: got '" + popDownAutoCompList.getNode().popupOpen + "', expected 'TRUE'"

(In reply to comment #7) > >+ controller.waitFor(function() { > >+ return popDownAutoCompList.getNode().popupOpen == true; > >+ }, TIMEOUT, 100, "Autocomplete popup is open"); Also the message has to be the second parameter, and you can get rid of the timeout and delay when those are equal to 5000 vs. 100.

Comment on attachment 533222 [details] [diff] [review] patch v3.0 >+ controller.waitFor(function() { >+ return popDownAutoCompList.getNode().popupOpen === true; >+ }, "Autocomplete popup is open: got '" + popDownAutoCompList.getNode().popupOpen + "', expected 'TRUE'"); >+ >+ controller.waitFor(function() { >+ return popDownAutoCompList.getNode().popupOpen; >+ }, "Popup Open - got '" + popDownAutoCompList.getNode().popupOpen + "'expected 'true'"); These appear to be duplicates. Can you please combine them? Keep the return statement from the second waitFor() and the message from the first waitFor(). For example... controller.waitFor(function() { return popDownAutoCompList.getNode().popupOpen; }, "Autocomplete popup is open: got '" + popDownAutoCompList.getNode().popupOpen + "', expected 'TRUE'"); >+ controller.waitFor(function() { >+ return popDownAutoCompList.getNode().popupOpen === true; >+ }, "Autocomplete popup is open: got '" + popDownAutoCompList.getNode().popupOpen + "', expected 'TRUE'"); >+ >+ controller.waitFor(function() { >+ return popDownAutoCompList.getNode().popupOpen; >+ }, "Popup Open - got '" + popDownAutoCompList.getNode().popupOpen + "'expected 'true'"); These appear to be duplicates as well. Please make a similar change as above. Sorry I didn't catch that in my last review.

And a little update. When you are using the message of the first wiatFor it has to be 'true' and not 'TRUE'.

Comment on attachment 533573 [details] [diff] [review] patch v4.0 Review of attachment 533573 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me -- over to Henrik for final review.

Comment on attachment 533573 [details] [diff] [review] patch v4.0 No, as discussed please also use Geo. I cannot handle all of the final reviews across the different projects.

Comment on attachment 533573 [details] [diff] [review] patch v4.0 Looks fine. I'm happy to see these isolated. r+ Am I landing these or is Anthony?

Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/797be573799b (default) http://hg.mozilla.org/qa/mozmill-tests/rev/12b4b0f22ca8 (aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/da24685a5690 (beta) (In reply to comment #15) > Am I landing these or is Anthony? Whoever did the super-review and has checkin permissions. Only if you can't land it immediately add the checkin-needed keyword. Leaving open for Litmus test updates. Please close once those have been updated.

Updated https://litmus.mozilla.org/show_test.cgi?id=15517 Updated https://litmus.mozilla.org/show_test.cgi?id=15518 Setting RESOLVED FIXED