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

Status: RESOLVED FIXED

(Thunderbird :: Testing Infrastructure, defect)

Description

[:aceman]

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.

Comment 1

[:aceman]

Attachment: patch

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 3

[:aceman]

(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

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 5

[:aceman]

Attachment: patch v2

Done.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/2e4f6c68fce4