Closed Bug 872444 Opened 8 years ago Closed 7 years ago

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

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(firefox23 wontfix, firefox24 fixed, firefox25 fixed, firefox26 fixed, firefox27 fixed, firefox-esr17 fixed)

RESOLVED FIXED
Tracking Status
firefox23 --- wontfix
firefox24 --- fixed
firefox25 --- fixed
firefox26 --- fixed
firefox27 --- fixed
firefox-esr17 --- fixed

People

(Reporter: AndreeaMatei, Assigned: alexdovbysh)

Details

(Whiteboard: [endurance][mentor=andreea.matei][lang=js][good first bug])

Attachments

(3 files, 6 obsolete files)

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.
Whiteboard: [enhancement][mentor=andreea.matei][lang=js][good first bug]
I would not put this into the enhancement bucket but as a possible candidate for test failures.
Summary: Add teardown() method in order to clean up, if necessary → Add teardown() method to 'endurance/testPrivateBrowsing_OpenAndClose/test1.js' to clean up, if necessary
Whiteboard: [enhancement][mentor=andreea.matei][lang=js][good first bug] → [endurance][mentor=andreea.matei][lang=js][good first bug]
Attached patch add_teardownModule_method (obsolete) — Splinter Review
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.
Attachment #767957 - Flags: review?
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
Attachment #767957 - Flags: review? → review-
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!
Summary: Add teardown() method to 'endurance/testPrivateBrowsing_OpenAndClose/test1.js' to clean up, if necessary → Add teardown() method in tests that are missing it, in order to clean up
Attached patch add_teardownModule_method_v0.2 (obsolete) — Splinter Review
trailing whitespaces deleted.
added changes to /functional/testPrivateBrowsing/testPrivateDownlodPanel.js. Replaced 'pb' by 'pbWindow'.
This is a file that misleads me xd.
Attachment #767957 - Attachment is obsolete: true
Attachment #768246 - Flags: review?
I dont see your post. I checked only directory endurance and yes I'll check for others.
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).
Attachment #768246 - Attachment is obsolete: true
Attachment #768246 - Flags: review?
Attachment #768859 - Flags: review?(andreea.matei)
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.
Attachment #768859 - Flags: review?(andreea.matei) → review-
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.
Attachment #770693 - Flags: review?(andreea.matei)
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.
Attachment #770693 - Flags: review?(andreea.matei) → review+
Assignee: nobody → alexdovbysh
Status: NEW → ASSIGNED
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!
Sure, I will check this.
Attached patch mozilla-release.patch (obsolete) — Splinter Review
conflicts solved for release branch
Attachment #786264 - Flags: review?(andreea.matei)
Attached patch mozilla-esr17.patch (obsolete) — Splinter Review
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
Attachment #786275 - Flags: review?(andreea.matei)
Attached patch mozilla-release.patch (obsolete) — Splinter Review
Attachment #786264 - Attachment is obsolete: true
Attachment #786264 - Flags: review?(andreea.matei)
Attachment #786309 - Flags: review?(andreea.matei)
Attached patch mozilla-esr17.patch (obsolete) — Splinter Review
some changes, endurance runtest for esr-17

http://mozmill-crowd.blargon7.com/#/endurance/report/bccc2ea9ac1e478fa074c699d892a74b
Attachment #786275 - Attachment is obsolete: true
Attachment #786275 - Flags: review?(andreea.matei)
Attachment #786311 - Flags: review?(andreea.matei)
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.
Attachment #786309 - Flags: review?(andreea.matei) → review-
Attachment #786311 - Flags: review?(andreea.matei) → review-
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
Flags: needinfo?(alexdovbysh)
(In reply to lsblakk@mozilla.com [: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.
Daniel, this bug is not about Firefox but Mozmill Tests. So please do not change our flags on behalf of your security work. Thanks.
Andreea, seems like we don't get a reply from Oleksandr. Can someone finish this bug off?
Flags: needinfo?(alexdovbysh)
Sure. Mario will finish here.
Attached patch ESR17_v2Splinter Review
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
Attachment #786309 - Attachment is obsolete: true
Attachment #786311 - Attachment is obsolete: true
Attachment #816581 - Flags: review?(andrei.eftimie)
Attachment #816581 - Flags: review?(andreea.matei)
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.
Attachment #816581 - Flags: review?(andrei.eftimie)
Attachment #816581 - Flags: review?(andreea.matei)
Attachment #816581 - Flags: review+
Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/c36cb52c42d4 (mozilla-esr17)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.