641574 - Panorama tests require tabview.reset() in teardownModule

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Something we missed with the recent tests is to make sure Panorama tests end cleanly. This bug is to make sure all existing tests do so by including tabview.reset() in their teardownModule().

Comment 1

[Aaron Train [:aaronmt]]

Ian, can we get a definition of what  contentWindow.UI.reset(), does? I don't think every test will need a reset call unless direct manipulation (resize, move, stack, create) of tab group view has occurred.

Comment 2

[u279076]

Attachment: Patch v1 [checked-in]

Call tabView.reset() in teardownModule() of all Panorama tests.  I've also included a small refactor to testToggleTabView to remove ambiguity between the tabView module and tabView controller.

Comment 3

[Aaron Train [:aaronmt]]

Comment on attachment 519210 [details] [diff] [review]
Patch v1 [checked-in]

From reading the doc [1], it would make sense to call reset() when one wants to restore the default state of Tab Groups (i.e, one default group), after manipulation has occurred, and does not automatically close additional tabs. 

http://hg.mozilla.org/labs/tabcandy/raw-file/tip/content/doc/files2/ui-js.html#UI.reset

The two landed tests don't do actual manipulations of groups, so adding these calls in are unnecessary.

Comment 4

[u279076]

Comment on attachment 519210 [details] [diff] [review]
Patch v1 [checked-in]

Henrik, can you give us feedback on this patch and Aaron's concerns in comment 3? Thanks

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Even tests do not add other groups those tests have to ensure that the tab group view gets closed. tabView.reset() will handle that internally. And yes it does not mess with the opened tabs, because that's really a clean-up routine from the tabs module.

Comment 6

[Ian Gilman [:iangilman]]

(In reply to comment #5)
> Even tests do not add other groups those tests have to ensure that the tab
> group view gets closed. tabView.reset() will handle that internally. And yes it
> does not mess with the opened tabs, because that's really a clean-up routine
> from the tabs module.

Do you mean that .reset() hides Panorama? It does not.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #6)
> Do you mean that .reset() hides Panorama? It does not.

No, I didn't. See the our current implementation:
http://hg.mozilla.org/qa/mozmill-tests/file/1ccaf9638b3b/lib/tabview.js#l112

Comment 8

[u279076]

Comment on attachment 519210 [details] [diff] [review]
Patch v1 [checked-in]

Aaron, given the context of recent comments, can you please re-review this patch?

Thanks.

Comment 9

[u279076]

Comment on attachment 519210 [details] [diff] [review]
Patch v1 [checked-in]

Thanks Aaron -- over to Henrik for follow up.

Comment 10

[u279076]

Comment on attachment 519210 [details] [diff] [review]
Patch v1 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/9e12e280ad7f [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/2b062ed37b71 [mozilla2.0]