872444 - Add teardown() method in tests that are missing it, in order to clean up

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Andreea Matei [:AndreeaMatei]]

I've noticed that test endurance/testPrivateBrowsing_OpenAndClose/test1.js does not have a teardown() method in order to make sure all pb windows are closed. We should add one.
Further than that, I think we should look for other cases, if any.

Comment 1

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

I would not put this into the enhancement bucket but as a possible candidate for test failures.

Comment 2

[Oleksandr]

Attachment: add_teardownModule_method

First of all, Andreea Matei thanks for your help at irc.
well, for better understanding of this .js file i would like to take a look at aModule class, but i cant find where is defined. Even so this patch should work.

Comment 3

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 767957 [details] [diff] [review]
add_teardownModule_method

Review of attachment 767957 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Olek, there are just 2 thing to edit before we can land this. 
Regarding aModule, it's not a class that you can find, is just a parameter in order to have the object instances declared globally.

Could you please add my email to the r? textbox when you add a patch? That way I'm notified I have to make a review :) Thanks!

::: tests/endurance/testPrivateBrowsing_OpenAndClose/test1.js
@@ +11,5 @@
>  
>  function setupModule(aModule) {
>    aModule.controller = mozmill.getBrowserController();
>    aModule.enduranceManager = new Endurance.EnduranceManager(aModule.controller);
> +  aModule.pb = new privateBrowsing.PrivateBrowsingWindow();

I would leave this as pbWindow, I feel is clearer and usually we use the class name like "enduranceManager" or "tabBrowser". I think it was misleading for you from the other tests that use pb instead. I'll file a bug for refactoring this (or if you want to do that, would be great).

@@ +25,2 @@
>  function testOpenAndClosePrivateBrowsingWindow() {
> +  enduranceManager.run(function () {    

You have added some trailing whitespaces

Comment 4

[Andreea Matei [:AndreeaMatei]]

One more thing Olek, could you please check for other tests that don't have teardownModule()? I think I saw one in functional/tabbedBrowsing/testCloseTab.js:
http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testTabbedBrowsing/testCloseTab.js

Not sure if there are others as well. Thanks!

Comment 5

[Oleksandr]

Attachment: add_teardownModule_method_v0.2

trailing whitespaces deleted.
added changes to /functional/testPrivateBrowsing/testPrivateDownlodPanel.js. Replaced 'pb' by 'pbWindow'.
This is a file that misleads me xd.

Comment 6

[Oleksandr]

I dont see your post. I checked only directory endurance and yes I'll check for others.

Comment 7

[Oleksandr]

Attachment: add_teardownModule_method

I left some files without teardown() method in functional/testSecurity and /functional/restartTests but i don't sure if it's necessary to add it.  (I'm doubting what this method would contain in those cases).

Comment 8

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 768859 [details] [diff] [review]
add_teardownModule_method

Review of attachment 768859 [details] [diff] [review]:
-----------------------------------------------------------------

Giving that you modify multiple tests, could you please run at least a functional testrun with your patch applied?
Here's how you can do that, you'll only need to add the --repository=path_to_your_repo:
https://developer.mozilla.org/en-US/docs/Mozmill_Tests#Running_Mozmill_tests

That's how you can make sure you've not introduced any regressions to the other tests.

::: tests/endurance/testPrivateBrowsing_OpenAndClose/test1.js
@@ +1,1 @@
> +/* This Source C}ode Form is subject to the terms of the Mozilla Public

You have introduced a character here.

@@ +13,5 @@
>    aModule.controller = mozmill.getBrowserController();
>    aModule.enduranceManager = new Endurance.EnduranceManager(aModule.controller);
> +  aModule.pbWindow = new privateBrowsing.PrivateBrowsingWindow();
> +  aModule.tabBrowser = new Tabs.tabBrowser(aModule.controller);
> +  aModule.tabBrowser.closeAllTabs();

We need to separate blocks of declarations from the ones with method calls.

::: tests/functional/testSearch/testSearchSuggestions.js
@@ +30,5 @@
>  }
>  
> +function teardownModule(aModule) {
> +  aModule.searchBar.clear();
> +  aModule.searchBar.restoreDefaultEngines();

Great catch with the clearing of the search text, but the second I don't think we need it as we're not installing any other engine in this test.

Comment 9

[Oleksandr]

Attachment: add_teardownModule_method_v0.3

Here result of the functional testrun. I also run all the tests I've modified

$ ./testrun_functional.py --repository=/home/oleksandr/mozmill-tests/ ~/mozilla-central/
*** Platform: Linux Ubuntu 12.04 64bit
*** Cloning repository to '/tmp/tmpH6BKXZ.mozmill-tests'
requesting all changes
adding changesets
adding manifests
adding file changes
added 3247 changesets with 9486 changes to 775 files (+4 heads)
updating to branch default
407 files updated, 0 files merged, 0 files removed, 0 files unresolved
File type .exe not supported.
*** Removing repository '/tmp/tmpH6BKXZ.mozmill-tests'
Traceback (most recent call last):
  File "./testrun_functional.py", line 51, in <module>
    main()
  File "./testrun_functional.py", line 46, in main
    FunctionalTestRun().run()
  File "/home/oleksandr/mozmill-automation/libs/testrun.py", line 439, in run
    raise self.last_exception
Exception: File type .exe not supported.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 770693 [details] [diff] [review]
add_teardownModule_method_v0.3

Review of attachment 770693 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Olek!

I did a functional testrun for this:
http://mozmill-crowd.blargon7.com/#/functional/report/a1b02004612785c13cf7c6bf1eb363b0
http://mozmill-crowd.blargon7.com/#/functional/report/a1b02004612785c13cf7c6bf1eb30adb

And an endurance one:
http://mozmill-crowd.blargon7.com/#/endurance/report/a1b02004612785c13cf7c6bf1eb26b01

It did not worked for you most likely due to the mozilla-central folder. Please download from here the archive for the latest nightly and extract it. Then you just give the patch to firefox/firefox.
ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central/

For the commit message you can drop the <> for the bug and have r=amatei as before, like this:
Bug 872444 - Added teardownModule() methods and changed pb in pbWindow in testPrivateDownlodPanel.js. r=amatei

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/93b5df4a0d27 (default)

We will need this on the other branches as well.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Hi Olek! Any chance you could check for the other branches?

For each, you just have to clone a repository, do "hg up -C branch_name" and apply the patch. If it applies cleanly, great! If not, you have to solve the conflicts manually and export the patch like "branch_name.patch" so you keep track of them.

The branch names are: mozilla-aurora, mozilla-beta, mozilla-release and mozilla-esr17.

Thanks!

Comment 12

[Oleksandr]

Sure, I will check this.

Comment 13

[Oleksandr]

Attachment: mozilla-release.patch

conflicts solved for release branch

Comment 14

[Oleksandr]

Attachment: mozilla-esr17.patch

conflicts solved for mozilla-esr17.
Nightly and aurora already had a patch, and beta has not conflicts.

Here results of functional testrun for aurora and esr17. And i cant find a firefox beta for linux 64bit xD.

http://mozmill-crowd.blargon7.com/#/functional/report/bccc2ea9ac1e478fa074c699d8807d48

http://mozmill-crowd.blargon7.com/#/functional/report/bccc2ea9ac1e478fa074c699d8822099

Comment 15

[Oleksandr]

Attachment: mozilla-release.patch

Comment 16

[Oleksandr]

Attachment: mozilla-esr17.patch

some changes, endurance runtest for esr-17

http://mozmill-crowd.blargon7.com/#/endurance/report/bccc2ea9ac1e478fa074c699d892a74b

Comment 17

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 786309 [details] [diff] [review]
mozilla-release.patch

Review of attachment 786309 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Olek. We need some small changes to both patches, so it will remain in harmony with the landed patch.
I can see that you didn't fixed the conflicts but just write the changes again, probably that's why you missed them. :)

In general is regarding our 'a' prefix to parameters, we have a working bug now about it (bug 883120) and I wouldn't want to add more work there. And then we use it before calling a method, in setup and teardown, as we did with your first patch.

Thanks!

::: tests/endurance/testFlash_SWFVideoEmbed/test1.js
@@ +31,5 @@
>    }
>  }
>  
> +function teardownModule(aModule) {
> +  tabBrowser.closeAllTabs();

Please add aModule here, like you did in the checked in patch:
aModule.tabBrowser.closeAllTabs();

::: tests/endurance/testFlash_SWFVideoObject/test1.js
@@ +32,5 @@
>  }
>  
> +function teardownModule(aModule) {
> +  tabBrowser.closeAllTabs();
> +}

Same here.

::: tests/endurance/testPrivateBrowsing_OpenAndClose/test1.js
@@ +17,5 @@
>  }
>  
> +function teardownModule(aModule) {
> +  pbWindow.close(true);
> +  tabBrowser.closeAllTabs();

aModule here as well.

@@ +24,3 @@
>  function testOpenAndClosePrivateBrowsingWindow() {
>    enduranceManager.run(function () {
>      var pbWindow = new privateBrowsing.PrivateBrowsingWindow();

We can remove this now that's declared in setup.

::: tests/functional/testPreferences/testRemoveAllCookies.js
@@ +19,5 @@
>  
>    Services.cookies.removeAll();
>  }
>  
> +var teardownModule = function(module) {

Please change the parameter name to aModule. We use 'a' prefix for all parameter, there's a bug that is changing it where we missed it and this would introduce new regressions there.

::: tests/functional/testSearch/testSearchSuggestions.js
@@ +28,5 @@
>  }
>  
> +function teardownModule() {
> +  searchBar.clear();
> +}

Please add aModule as parameter and before the searchBar.

::: tests/functional/testTabbedBrowsing/testCloseTab.js
@@ +14,5 @@
>  }
>  
> +var teardownModule = function(module) {
> +  tabBrowser.closeAllTabs();
> +}

Same here, aModule.

Comment 18

[Andreea Matei [:AndreeaMatei]]

Transplanted the r+ patch to beta as well:
http://hg.mozilla.org/qa/mozmill-tests/rev/d7c5c1253274 (beta)

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Do you have an updated patch with r+ for ESR17 branch?  If you want to land there you'll need to nominate for mozilla-esr

Comment 20

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

(In reply to [e-mail] [:lsblakk] from comment #19)
> Do you have an updated patch with r+ for ESR17 branch?  If you want to land
> there you'll need to nominate for mozilla-esr

Lukas, this is the mozmill-tests component and we handle all patch landings differently than you for Firefox. There is no nomination needed.

Comment 21

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

Daniel, this bug is not about Firefox but Mozmill Tests. So please do not change our flags on behalf of your security work. Thanks.

Comment 22

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

Andreea, seems like we don't get a reply from Oleksandr. Can someone finish this bug off?

Comment 23

[Andreea Matei [:AndreeaMatei]]

Sure. Mario will finish here.

Comment 24

[mario garbi]

Attachment: ESR17_v2

The patch got merged along with Beta on Release and only esr17 needed a backport. I haven't updated the teardownModule with (aModule) as we have a refactor bug that will do that - Bug 883120. 

Linux report:
http://mozmill-crowd.blargon7.com/#/functional/report/ec449c026814fd64783d738c0268872d

Comment 25

[Andrei Eftimie]

Comment on attachment 816581 [details] [diff] [review]
ESR17_v2

Review of attachment 816581 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me.
Since the modified tests are in different folders, please run a functional, a remote and an endurance testrun on ESR17.

I'll land this once we have these reports.

Comment 26

[mario garbi]

Reports for ESR17:

Functional:
http://mozmill-crowd.blargon7.com/#/functional/report/ec449c026814fd64783d738c02983391
http://mozmill-crowd.blargon7.com/#/functional/report/ec449c026814fd64783d738c029930c2
Remote:
http://mozmill-crowd.blargon7.com/#/remote/report/ec449c026814fd64783d738c02999394
http://mozmill-crowd.blargon7.com/#/remote/report/ec449c026814fd64783d738c029a357b
Endurance:
http://mozmill-crowd.blargon7.com/#/endurance/report/ec449c026814fd64783d738c02967c61

Comment 27

[Andrei Eftimie]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/c36cb52c42d4 (mozilla-esr17)