Closed
Bug 817390
Opened 12 years ago
Closed 12 years ago
make click_account_tree_row() in test-account-manager-helpers.js check sanity of its arguments
Categories
(Thunderbird :: Testing Infrastructure, defect)
Thunderbird
Testing Infrastructure
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 20.0
People
(Reporter: aceman, Assigned: aceman)
References
Details
Attachments
(1 file, 1 obsolete file)
18.28 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
Make click_account_tree_row() in test-account-manager-helpers.js check sanity of its arguments. This will enable to remove all those "assert_not_equals(accountRow, -1);" checks I added in bug 805953. Also make get_account_tree_row() dump a warning if it didn't find the wanted account/pane. This should allow for a much better log output about why the click failed.
Attachment #687516 -
Flags: review?(mconley)
Comment 2•12 years ago
|
||
Comment on attachment 687516 [details] [diff] [review] patch Review of attachment 687516 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/test/mozmill/account/test-account-settings-infrastructure.js @@ +87,5 @@ > > accountRow = get_account_tree_row(gPopAccount.key, "am-server.xul", amc); > click_account_tree_row(amc, accountRow); > > + iframe = amc.e("contentFrame").contentDocument; I'd like a comment explaining why we need to re-assign iframe. @@ +187,5 @@ > > accountRow = get_account_tree_row(gPopAccount.key, "am-addressing.xul", amc); > click_account_tree_row(amc, accountRow); > > + iframe = amc.e("contentFrame").contentDocument; Same as above - I'd like a quick comment to explain the re-assignment of iframe. @@ +259,5 @@ > > accountRow = get_account_tree_row(gPopAccount.key, "am-server.xul", amc); > click_account_tree_row(amc, accountRow); > > + iframe = amc.e("contentFrame").contentDocument; Same as before - re: comment. ::: mail/test/mozmill/shared-modules/test-account-manager-helpers.js @@ +107,5 @@ > * @param aAccountKey The key of the account to return. > * If 'null', the SMTP pane is returned. > * @param aPaneId The ID of the account settings pane to select. > + * > + * @return The row index of the account and pane. If it was not found return -1. Is there ever a case where we *want* to return -1? Why not throw an Error instead?
(In reply to Mike Conley (:mconley) from comment #2) > Comment on attachment 687516 [details] [diff] [review] > patch > > Review of attachment 687516 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/test/mozmill/account/test-account-settings-infrastructure.js > @@ +87,5 @@ > > > > accountRow = get_account_tree_row(gPopAccount.key, "am-server.xul", amc); > > click_account_tree_row(amc, accountRow); > > > > + iframe = amc.e("contentFrame").contentDocument; > > I'd like a comment explaining why we need to re-assign iframe. Because the contentDocument was replaced when switching panes. The new pane is loaded via document.getElementById("contentFrame").webNavigation.loadURI . The contentDocument is lost. Or does it still work in your tests? > ::: mail/test/mozmill/shared-modules/test-account-manager-helpers.js > @@ +107,5 @@ > > * @param aAccountKey The key of the account to return. > > * If 'null', the SMTP pane is returned. > > * @param aPaneId The ID of the account settings pane to select. > > + * > > + * @return The row index of the account and pane. If it was not found return -1. > > Is there ever a case where we *want* to return -1? Why not throw an Error > instead? It is in a comment in the patch: "Do not throw as callers may intentionally just check if a row exists." Imagine a test that checks if IM account does NOT have a am-junk.xul pane...
Comment 4•12 years ago
|
||
Comment on attachment 687516 [details] [diff] [review] patch r=me with the comment about the iframe re-assignment. I'm dropping the other issue I raised (throwing an Error for get_account_tree_row), because there are legitimate cases where the caller might want to check if a row doesn't exist without erroring out.
Attachment #687516 -
Flags: review?(mconley) → review+
Attachment #687516 -
Attachment is obsolete: true
Keywords: checkin-needed
Comment 6•12 years ago
|
||
https://hg.mozilla.org/comm-central/rev/2e4f6c68fce4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0
You need to log in
before you can comment on or make changes to this bug.
Description
•