use CommonJS for shared modules

VERIFIED FIXED

Status

Testing Graveyard
Mozmill
VERIFIED FIXED
8 years ago
2 years ago

People

(Reporter: harth, Assigned: harth)

Tracking

Details

(Whiteboard: [mozmill-2.0+][mozmill-1.5.1+])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
we should use CommonJS for the Mozmill shared modules, to re-use module loading code and also to allow people to import modules for different directories and give them whatever namespace they want.
(Assignee)

Comment 1

8 years ago
Created attachment 470001 [details] [diff] [review]
CommonJS shared modules

I have this implemented. It's a really unobtrusive change. It uses the Jetpack "securable-module" loader with only a few changes. I left support for the current shared module system in for now. In my commonjs branch:

http://github.com/harthur/mozmill/compare/commonjs
Assignee: nobody → fayearthur+bugs
(Assignee)

Comment 2

8 years ago
Created attachment 470096 [details] [diff] [review]
CommonJS shared modules, plus a test file

adding a test with some modules in different directories
Attachment #470096 - Flags: review?(ctalbert)
(Assignee)

Comment 3

8 years ago
Created attachment 470487 [details] [diff] [review]
CommonJS shared modules minus old shared module code

patch that takes out the old shared module code
Attachment #470001 - Attachment is obsolete: true
Attachment #470487 - Flags: review?(ctalbert)
(Assignee)

Updated

8 years ago
Blocks: 523931
(Assignee)

Updated

8 years ago
Blocks: 581004
(Assignee)

Updated

8 years ago
Blocks: 515341

Comment 4

8 years ago
Comment on attachment 470096 [details] [diff] [review]
CommonJS shared modules, plus a test file

This looks great.  My only nit is that I'd like to see a test that loads a module which contains a UTF-8 escaped string.  We've had issues with that in the past and so having a test for that will help to make sure we don't regress that behavior going forward.
Attachment #470096 - Flags: review?(ctalbert) → review+

Comment 5

8 years ago
Comment on attachment 470487 [details] [diff] [review]
CommonJS shared modules minus old shared module code

This looks good, but let's hold off on landing this until we have either:
(A): a patch that changes all the mozmill-tests to a common-js style loading for shared modules
(B): The mozmill unit test harness (for mozmill itself)

Which ever of those happens first are ok.  I'm thinking that B will happen first as A requires the qa guys to decide to branch for 2.0, which will take longer.

But I don't want to land it until then because it's going to break all our ability to test things until one of those two items land.  Alternatively, we can go ahead with the first patch now, and then land the removal of the existing shared-module loading stuff later.  Happy to do it that way, if you want.
Attachment #470487 - Flags: review?(ctalbert) → review+
(In reply to comment #5)
> This looks good, but let's hold off on landing this until we have either:
> (A): a patch that changes all the mozmill-tests to a common-js style loading
> for shared modules
> (B): The mozmill unit test harness (for mozmill itself)
> 
> Which ever of those happens first are ok.  I'm thinking that B will happen
> first as A requires the qa guys to decide to branch for 2.0, which will take
> longer.

I would propose to have a play ground with 2.0 stuff outside of the mozmill-tests repository to not conflict with the way we are handling branches. The other reason is that we have multiple branches (4.0, 3.6, 3.5) which will have to be converted. I would be in favor of doing that once (hopefully in a scripted fashion) when we officially upgrade to 2.0.
Status: NEW → ASSIGNED
Something I have discussed with Geo yesterday in short, wouldn't it be helpful to half-wise transform our current tests to use the CommonJS module by just placing the code inside mozmill-tests for now? We would still have one single statement which includes the common js module but all other inclusions we could do via require. I haven't tested it yet, so I wonder if that would work.
Comment on attachment 470487 [details] [diff] [review]
CommonJS shared modules minus old shared module code

>+++ b/mozmill/mozmill/extension/resource/modules/securable-module.js

Heather, please move the secure-modules.js file to resource/stdlib/. It's not a module we own. All external modules should really be stored under the above given folder.
Attachment #470487 - Flags: review-
(Assignee)

Comment 9

8 years ago
(In reply to comment #8)
> Comment on attachment 470487 [details] [diff] [review]
> CommonJS shared modules minus old shared module code
> 
> >+++ b/mozmill/mozmill/extension/resource/modules/securable-module.js
> 
> Heather, please move the secure-modules.js file to resource/stdlib/. It's not a
> module we own. All external modules should really be stored under the above
> given folder.

already checked in, can change later:
http://github.com/mozautomation/mozmill/commit/70e788d486a6b37cc1244663eb2c871222a6d464
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-2.0+]
Depends on: 598084
As discussed on IRC we will already have the Common JS module for Mozmill 1.5.x (eventually with a version change to 1.6). It will not replace the old collector code but would give us the change to already update all of our tests to use 'require' instead of the old crufty module inclusion. Also add-on authors could reference our shared modules and will not have to clone them.

Before we land it on any of the upcoming/existing 1.5.x hotfix branches we have to fix bug 598084.
Whiteboard: [mozmill-2.0+] → [mozmill-2.0+][mozmill-1.5.1?]
(Assignee)

Updated

8 years ago
Whiteboard: [mozmill-2.0+][mozmill-1.5.1?] → [mozmill-2.0+][mozmill-1.5.1+]
Blocks: 602365
Depends on: 602455
I run a couple of deeper tests for the Securable Module implementation in Mozmill. Beside bug 602455 I wasn't able to detect any other regression. It's great to load other modules via require and finally across different folders. Test authors for add-ons will be more than happy with Mozmill 1.5.1!

Also I really appreciate the willingness for a smooth transition to the new module loader, so nothing will get broken in fact of module loading when upgrading to Mozmill 2.0.

Marking as verified fixed.
Status: RESOLVED → VERIFIED
See Also: → bug 1121939
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.