918243 - Refactor javascript mutt tests declarations

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect, P3)

Description

[Andrei Eftimie]

We should unify how we declare the mutt tests.

All setupModule, teardownModule and testXXX who follow the old style:
> var setupModule = function (aModule) {

should adhere to the new style:
> function setupModule(aModule) {

This should be done across all js mutt tests found under mutt/mutt/tests/js

Comment 1

[Errietta Kostala [:errietta]]

Attachment: Proposed patch

Comment 2

[Andrei Eftimie]

Hi Errietta,

Thanks for the patch, I'll look at it shortly.

Comment 3

[Andrei Eftimie]

Comment on attachment 808616 [details] [diff] [review]
Proposed patch

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

Thanks for the patch Errietta!
There are 2 things to fix before we can land this, please see my comments.

Otherwise this looks good.

::: mutt/mutt/tests/js/testAssertions/testExpectAssert.js
@@ +91,4 @@
>  /**
>   * Tests for supported expect methods
>   */
> +function testExpect () {

As per our style guide:
https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide#Naming

Please remove the space between the name of the function and the parentheses.
This applies to all functions.

::: mutt/mutt/tests/js/testController/testDndChrome.js
@@ +4,4 @@
>  
>  const TEST_DATA = "chrome://mozmill/content/test/test.xul";
>  
> +function setupModule () {

Since we are touching this code, I think we should also update it to stop using global variables.

Please use aModule as a parameter in all `setupModule` and `teardownModule` functions you find, and use this as a namespace for all variables created in these functions.

As an example:
https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/js/testController/testEnterLeavePrivateBrowsing.js#L11

Comment 4

[Errietta Kostala [:errietta]]

Hi,

About your second comment. In mutt/mutt/tests/js/testFrame/testUserShutdown/testMultiple/test6.js there is   controller.startUserShutdown(1000, false);
controller.mainMenu.click("#menu_FileQuitItem"); in SetupModule. I should change that to aModule.controller too, right?

Comment 5

[Andrei Eftimie]

Yes, any variable instantiated in setupModule (like controller) should be instantiated and used with aModule.controller.

All testXXX functions have access to these variables directly, we want to stop polluting the global namespace, hence this change.

Comment 6

[Errietta Kostala [:errietta]]

Attachment: Another try!

Comment 7

[Errietta Kostala [:errietta]]

Attachment: patch

Sorry, i forgot to convert to hg

Comment 8

[Andrei Eftimie]

Comment on attachment 810277 [details] [diff] [review]
patch

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

This looks good.

I've noticed that we still have `setupTest` in places where we use `setupModule` now.
They should probably get updated as well.

But in some places they appear to be used differently.

Henrik, should we also change `setupTest` instances to `setupModule`?
I see they are being used in the machine state tests, we those should probably be left as they are now.
(eg. testCountTestPasses)

Should the other ones be changed to `setupModule` (in places where we don't use both `setupTest` and `setupModule`)?

::: mutt/mutt/tests/js/testFrame/testPersistedData.js
@@ +6,1 @@
>    controller = mozmill.getBrowserController();

I just noticed this. `setupTest` was used way back where we now use `setupModule`.
Please update this function as well.

::: mutt/mutt/tests/js/testFrame/testRestart/testApplicationDisconnect/test1.js
@@ +6,4 @@
>  // Application disconnect errors because of invalid frame objects
>  // if a test file restarts the browser
>  
> +function setupTest() {

Another one.

Comment 9

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

Comment on attachment 810277 [details] [diff] [review]
patch

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

It looks fine, indeed. But there are only two small issue to fix before you get my r+. Please check the details inline.

::: mutt/mutt/tests/js/testController/testContextMenu.js
@@ +10,3 @@
>  
>    // Create a new menu instance for the context menu
> +  aModule.contextMenu = controller.getMenu("#contentAreaContextMenu");

This has to be aModule.controller.getMenu(). You missed the prefix in a couple of cases. Please check them all.

::: mutt/mutt/tests/js/testFrame/testPersistedData.js
@@ +6,1 @@
>    controller = mozmill.getBrowserController();

What? No, this cannot be changed. As you can see this is a restart test and for each test we need the controller. setupModule() will only define it for the very first process.

::: mutt/mutt/tests/js/testFrame/testRestart/testApplicationDisconnect/test1.js
@@ +6,4 @@
>  // Application disconnect errors because of invalid frame objects
>  // if a test file restarts the browser
>  
> +function setupTest() {

No, that's also fully correct. Please don't propose those changes when you are not sure about the behavior of those methods.

Comment 10

[Errietta Kostala [:errietta]]

Attachment: patch

Comment 11

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

Comment on attachment 810720 [details] [diff] [review]
patch

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

It looks fine Errietta. There are only three small nits I would like to see fixed before the landing. Can you please update the patch?

::: mutt/mutt/tests/js/testFrame/testPersistedData.js
@@ +23,4 @@
>    expect.ok(!persisted.data, "User-defined object has been removed.");
>  }
>  
> +function teardownModule(aModule) {

We don't use aModule here, so you can remove it.

::: mutt/mutt/tests/js/testFrame/testRestart/testApplicationDisconnect/test1.js
@@ +10,1 @@
>    controller = mozmill.getBrowserController();

You will also use aModule here.

::: mutt/mutt/tests/js/testFrame/testRestart/testApplicationDisconnect/test2.js
@@ +6,4 @@
>  // Application disconnect errors because of invalid frame objects
>  // if a test file restarts the browser
>  
> +function setupTest() {

Same here.

Comment 12

[Errietta Kostala [:errietta]]

Hi,
There are other tests that have setupTest and do controller = ..., should I do that for those, too? Also, I see that other files have setupTest(aTest), should I name my parameter aTest or aModule there?

Comment 13

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

Yes, absolutely. aTest have been added most likely along time ago. But the parameter we use here is indeed our test module so aModule would be appropriate. Also if there are other tests which don't make use of it yet, it would be great if you could also cover those. Thanks Errietta!

Comment 14

[Errietta Kostala [:errietta]]

Attachment: patch

Comment 15

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

Comment on attachment 813983 [details] [diff] [review]
patch

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

The existing changes are looking fine now. But I did a run with grep "grep -r 'function ('" and it looks like you missed some of the declarations:

mutt/mutt/tests/js/testController/testScreenshot.js:var check_screenshot = function (aScreenshot, aName, aIsFile) {
mutt/mutt/tests/python/test_persisted_object.py:    var setupModule = function () {
mutt/mutt/tests/python/test_persisted_object.py:    let test = function () {
mutt/mutt/tests/python/js-modules/testExpectStack.js:var setupModule = function () {
mutt/mutt/tests/python/js-modules/testExpectStack.js:var test = function () {
mutt/mutt/tests/python/js-modules/useMozmill/testTestFails.js:var setupModule = function () {
mutt/mutt/tests/python/js-modules/useMozmill/testTestFails.js:var setupTest = function () {
mutt/mutt/tests/python/js-modules/useMozmill/testTestFails.js:var test = function () {
mutt/mutt/tests/python/js-modules/useMozmill/testTestPass.js:var setupModule = function () {
mutt/mutt/tests/python/js-modules/useMozmill/testTestPass.js:var setupTest = function () {
mutt/mutt/tests/python/js-modules/useMozmill/testTestPass.js:var test = function () {
mutt/mutt/tests/python/js-modules/testWaitForPassFrame.js:var testSleep = function () {
mutt/mutt/tests/python/js-modules/testWaitForPassFrame.js:var testWaitFor = function () {
mutt/mutt/tests/python/js-modules/testUTF-8.js:var setupModule = function () {
mutt/mutt/tests/python/js-modules/testUTF-8.js:var test = function () {
mutt/mutt/tests/python/js-modules/testFailureBackgroundThread.js:var setupModule = function (aModule) {
mutt/mutt/tests/python/js-modules/testFailureBackgroundThread.js:var testExceptionBackgroundThread = function () {
mutt/mutt/tests/python/test_screenshot_path.py:        var setupModule = function () {
mutt/mutt/tests/python/cli/test_profile_relative_path.py:        test = """var test = function () { };"""

Please get those updated with this patch too. Once done the patch will be ready. Thanks Errietta!

Comment 16

[Errietta Kostala [:errietta]]

Attachment: patch

Comment 17

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

Comment on attachment 816112 [details] [diff] [review]
patch

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

That looks great. I will get it landed soon.

Comment 18

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

Landed on master as:
https://github.com/mozilla/mozmill/commit/f9e33250dca871f39684f7e1e3d2b3170dfb90f7

Thanks Errietta! Please let me know if you have interests on working on other bugs even more challenging.