Last Comment Bug 817390 - make click_account_tree_row() in test-account-manager-helpers.js check sanity of its arguments
: make click_account_tree_row() in test-account-manager-helpers.js check sanity...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Testing Infrastructure (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 20.0
Assigned To: :aceman
:
Mentors:
Depends on: 805953
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-02 06:50 PST by :aceman
Modified: 2012-12-10 05:16 PST (History)
1 user (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (17.92 KB, patch)
2012-12-02 07:30 PST, :aceman
mconley: review+
Details | Diff | Splinter Review
patch v2 (18.28 KB, patch)
2012-12-07 14:49 PST, :aceman
acelists: review+
Details | Diff | Splinter Review

Description :aceman 2012-12-02 06:50:20 PST
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 2012-12-02 07:30:21 PST
Created attachment 687516 [details] [diff] [review]
patch
Comment 2 Mike Conley (:mconley) - (Needinfo me!) 2012-12-07 08:39:23 PST
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 2012-12-07 14:17:44 PST
(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) - (Needinfo me!) 2012-12-07 14:35:33 PST
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 2012-12-07 14:49:21 PST
Created attachment 689953 [details] [diff] [review]
patch v2

Done.
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-12-10 05:16:17 PST
https://hg.mozilla.org/comm-central/rev/2e4f6c68fce4

Note You need to log in before you can comment on or make changes to this bug.