Closed Bug 599142 Opened 14 years ago Closed 14 years ago

Split testClearFormHistory.js into two modules

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: u279076, Assigned: AlexLakatos)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Product: Testing → Mozilla QA
Assignee: nobody → alex.lakatos
Status: NEW → ASSIGNED
Alex Lakatos changed story state to started in Pivotal Tracker
Attached patch patch v1.0 (obsolete) — Splinter Review
Attachment #531907 - Flags: review?(anthony.s.hughes)
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.
Attachment #531907 - Flags: review?(anthony.s.hughes) → review-
Attached patch patch v2.0 (obsolete) — Splinter Review
Attachment #531907 - Attachment is obsolete: true
Attachment #532896 - Flags: review?(anthony.s.hughes)
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'"
Attachment #532896 - Flags: review?(anthony.s.hughes) → review-
(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.
Attached patch patch v3.0 (obsolete) — Splinter Review
Attachment #532896 - Attachment is obsolete: true
Attachment #533222 - Flags: review?(anthony.s.hughes)
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.
Attachment #533222 - Flags: review?(anthony.s.hughes) → review-
And a little update. When you are using the message of the first wiatFor it has to be 'true' and not 'TRUE'.
Attached patch patch v4.0Splinter Review
Attachment #533222 - Attachment is obsolete: true
Attachment #533573 - Flags: review?(anthony.s.hughes)
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.
Attachment #533573 - Flags: review?(hskupin)
Attachment #533573 - Flags: review?(anthony.s.hughes)
Attachment #533573 - Flags: 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.
Attachment #533573 - Flags: review?(hskupin) → review?(gmealer)
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?
Attachment #533573 - Flags: review?(gmealer) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: