make click_account_tree_row() in test-account-manager-helpers.js check sanity of its arguments

RESOLVED FIXED in Thunderbird 20.0

Status

Thunderbird
Testing Infrastructure
--
trivial
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 20.0
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

18.28 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 687516 [details] [diff] [review]
patch
Attachment #687516 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
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?
(Assignee)

Comment 3

5 years ago
(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 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+
(Assignee)

Comment 5

5 years ago
Created attachment 689953 [details] [diff] [review]
patch v2

Done.
Attachment #689953 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #687516 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/2e4f6c68fce4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.