999393 - The Assertion module is not loaded in included files via require

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Andrei Eftimie]

Attachment: testcase.zip

In included files via require the Assertion module is not loaded.

I've attached a testcase (It's a zip since we require 2 files):
- in testAssertInIncludedFile.js the `assert` object exists.
- in lib.js which is loaded as a module from within testAssertInIncludedFile, the assert object does not exist.

If you run the lib.js file directly it will not fail.

Expected results:
The assert object should be available in files loaded as modules.

Comment 1

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

Looks like we miss to set the property here?
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/frame.js#L539

Comment 2

[Andrei Eftimie]

Attachment: 0001-Bug-999393-Expose-assert-as-a-global-var-in-module.r.patch

We should include this in the next mozmill release.

Comment 3

[Andrei Eftimie]

Attachment: 0002-Bug-999393-Expose-assert-as-a-global-var-in-module.r.patch

I forgot to also include `expect` in the first patch.

Both `assert` and `expect` are now globally available.

Comment 4

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

Comment on attachment 8410891 [details] [diff] [review]
0002-Bug-999393-Expose-assert-as-a-global-var-in-module.r.patch

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

::: mozmill/mozmill/extension/resource/modules/frame.js
@@ +542,5 @@
>        rootPaths: [Services.io.newFileURI(file.parent).spec,
>                    "resource://mozmill/modules/"],
>        defaultPrincipal: "system",
> +      globals : { assert: assert,
> +                  expect: expect,

Please also update the lines above where we create new instances of the Assert and Expect class. That's not necessary given that we  have global instances already.

::: mutt/mutt/tests/js/testFrame/testRequire/manifest.ini
@@ +1,5 @@
>  [DEFAULT]
>  type = javascript
>  
>  [testRequire.js]
> +[testRequireLoadsAssert.js]

I would call it testRequireDefaultModules, so we can also check for other modules which are included by default like mozelement. For the latter please also add a small test.

::: mutt/mutt/tests/js/testFrame/testRequire/moduleD.js
@@ +3,5 @@
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +try {
> +  assert.ok(true, "assert is loaded");
> +  expect.ok(true, "expect is loaded");

I would do the expect test first, so we can test both in case of a failure.

@@ +7,5 @@
> +  expect.ok(true, "expect is loaded");
> +}
> +catch (e) {
> +  throw new Error("The Assertion Module is available in the included module: " + e);
> +}

I don't see why you need the try/catch here. Just make it a function please, which gets called from the test.

::: mutt/mutt/tests/js/testFrame/testRequire/testRequireLoadsAssert.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +

I miss the strict definition.

@@ +8,5 @@
> +    expect.ok(true, "expect is loaded");
> +  }
> +  catch (e) {
> +    throw new Error("The Assertion Module is available in the test file: " + e);
> +  }

Same here as for the submodule.

@@ +9,5 @@
> +  }
> +  catch (e) {
> +    throw new Error("The Assertion Module is available in the test file: " + e);
> +  }
> +  var module = require("moduleD");

As usual this has to be located at the top of the module.

Comment 5

[Andrei Eftimie]

Attachment: 0003-Bug-999393-Expose-assert-as-a-global-var-in-module.r.patch

Thanks for the quick review.

Fixed the requests, and some more comments inline:

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Comment on attachment 8410891 [details] [diff] [review]
> 0002-Bug-999393-Expose-assert-as-a-global-var-in-module.r.patch
> 
> Review of attachment 8410891 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozmill/mozmill/extension/resource/modules/frame.js
> @@ +542,5 @@
> >        rootPaths: [Services.io.newFileURI(file.parent).spec,
> >                    "resource://mozmill/modules/"],
> >        defaultPrincipal: "system",
> > +      globals : { assert: assert,
> > +                  expect: expect,
> 
> Please also update the lines above where we create new instances of the
> Assert and Expect class. That's not necessary given that we  have global
> instances already.

We actually don't have them set as globals directly:
https://github.com/mozilla/mozmill/blob/15c0dc97fde8f44ae3bd4cbf2ef576239b156ded/mozmill/mozmill/extension/resource/modules/frame.js#L509

We have them set on the module:
https://github.com/mozilla/mozmill/blob/15c0dc97fde8f44ae3bd4cbf2ef576239b156ded/mozmill/mozmill/extension/resource/modules/frame.js#L524-L533

And now we're setting them as globals in the loaded files via require.
I've tried setting them as globals directly, but for some reason it doesn't
work.

> ::: mutt/mutt/tests/js/testFrame/testRequire/moduleD.js
> @@ +3,5 @@
> > + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +try {
> > +  assert.ok(true, "assert is loaded");
> > +  expect.ok(true, "expect is loaded");
> 
> I would do the expect test first, so we can test both in case of a failure.

I've tried this, but it still won't show both error messages.
If we miss both assert and expect, it fails with `ReferenceError: expect is not
defined`. We still won't get 2 messages. We're not testing the validity of the
expect/assert calls, we're testing whether their names are available in the 
given context.

With this I've realised we can simplify the testing procedure. We only need to assert that the object exist, so I'm just referencing them without any action. If one of them is missing it will fail with the ReferenceError. Looks way cleaner now.

Comment 6

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

Comment on attachment 8410953 [details] [diff] [review]
0003-Bug-999393-Expose-assert-as-a-global-var-in-module.r.patch

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

You misunderstood my last review comment regarding global declarations. What I meant is, please also update those lines:

https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/frame.js#L524
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/frame.js#L533

With that addition you will have my r+.

Comment 7

[Andrei Eftimie]

Attachment: 0004-Bug-999393-Expose-assert-as-a-global-var-in-module.r.patch

As, I see what you meant. Updated the patch accordingly.
Thanks

Comment 8

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

Comment on attachment 8411686 [details] [diff] [review]
0004-Bug-999393-Expose-assert-as-a-global-var-in-module.r.patch

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

Looks fine. I will get this landed tomorrow.

Comment 9

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

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

Thanks Andrei!

Comment 10

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

Andrei, given that we have to release a 2.0.7 release of Mozmill, could you please check if that patch also works on the hotfix-2.0 branch? I would like to get this in too.

Comment 11

[Andrei Eftimie]

Indeed it applies cleanly and works fine on hotfix-2.0

Comment 12

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

Thanks Andrei. I cherry-picked it for 2.0.7:
https://github.com/mozilla/mozmill/commit/8e56dcf1ff655bd8daf449351da897eeb4879fe2