Closed
Bug 599142
Opened 14 years ago
Closed 14 years ago
Split testClearFormHistory.js into two modules
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: u279076, Assigned: AlexLakatos)
References
Details
Attachments
(1 file, 3 obsolete files)
10.86 KB,
patch
|
u279076
:
review+
gmealer
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•14 years ago
|
||
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 | ||
Updated•14 years ago
|
Assignee: nobody → alex.lakatos
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•14 years ago
|
||
* testSaveFormInformation.js - https://litmus.mozilla.org/show_test.cgi?id=15518
* testClearFormHistory.js - https://litmus.mozilla.org/show_test.cgi?id=15517
Assignee | ||
Comment 4•14 years ago
|
||
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-
Assignee | ||
Comment 6•14 years ago
|
||
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-
Comment 8•14 years ago
|
||
(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.
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #532896 -
Attachment is obsolete: true
Attachment #533222 -
Flags: review?(anthony.s.hughes)
Reporter | ||
Comment 10•14 years ago
|
||
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-
Comment 11•14 years ago
|
||
And a little update. When you are using the message of the first wiatFor it has to be 'true' and not 'TRUE'.
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #533222 -
Attachment is obsolete: true
Attachment #533573 -
Flags: review?(anthony.s.hughes)
Reporter | ||
Comment 13•14 years ago
|
||
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 14•14 years ago
|
||
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+
Comment 16•14 years ago
|
||
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.
Assignee | ||
Comment 17•14 years ago
|
||
Updated https://litmus.mozilla.org/show_test.cgi?id=15517
Updated https://litmus.mozilla.org/show_test.cgi?id=15518
Setting RESOLVED FIXED
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•