The Assertion module is not loaded in included files via require

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: andrei, Assigned: andrei)

Tracking

unspecified
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-2.0.7])

Attachments

(2 attachments, 3 obsolete attachments)

Posted file 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.
We should include this in the next mozmill release.
Assignee: nobody → andrei.eftimie
Status: NEW → ASSIGNED
Attachment #8410868 - Flags: review?(hskupin)
I forgot to also include `expect` in the first patch.

Both `assert` and `expect` are now globally available.
Attachment #8410868 - Attachment is obsolete: true
Attachment #8410868 - Flags: review?(hskupin)
Attachment #8410891 - Flags: review?(hskupin)
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.
Attachment #8410891 - Flags: review?(hskupin) → review-
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.
Attachment #8410891 - Attachment is obsolete: true
Attachment #8410953 - Flags: review?(hskupin)
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+.
Attachment #8410953 - Flags: review?(hskupin) → review+
As, I see what you meant. Updated the patch accordingly.
Thanks
Attachment #8410953 - Attachment is obsolete: true
Attachment #8411686 - Flags: review?(hskupin)
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.
Attachment #8411686 - Flags: review?(hskupin) → review+
Landed on master:
https://github.com/mozilla/mozmill/commit/da5d0291712b7aece25216f27040847fc37a17fc

Thanks Andrei!
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [mozmill-2.1]
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.
Flags: needinfo?(andrei.eftimie)
Indeed it applies cleanly and works fine on hotfix-2.0
Flags: needinfo?(andrei.eftimie)
Thanks Andrei. I cherry-picked it for 2.0.7:
https://github.com/mozilla/mozmill/commit/8e56dcf1ff655bd8daf449351da897eeb4879fe2
Blocks: 980801
Whiteboard: [mozmill-2.1] → [mozmill-2.0.7]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.