setupModule and teardownModule leak variables declared on the aModule argument

RESOLVED INVALID

Status

Testing Graveyard
Mozmill
RESOLVED INVALID
4 years ago
a year ago

People

(Reporter: Andrei Eftimie, Unassigned)

Tracking

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Created attachment 8440619 [details]
test.js

Variables declared in setupModule as properties of the argument variable (`aModule`) are also available stand-alone in both setupModule and teardownModule.

These should be available in the test functions, but not in the setup and teardown module functions.

Relevant code if in frame.js, I have looked over it but am to unfamiliar with it to point where the problem is.

I've attached a testcase. In both setupModule and teardownModule I would expect `x` to be `undefined`.
(In reply to Andrei Eftimie from comment #0)
> These should be available in the test functions, but not in the setup and
> teardown module functions.

No, that's not correct. aModule is the identical module you are in here. So why should those variables not be accessible? All of our tests would have been failed before we made use of it.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → INVALID
(Reporter)

Comment 2

4 years ago
(In reply to Henrik Skupin (:whimboo) from comment #1)
> (In reply to Andrei Eftimie from comment #0)
> > These should be available in the test functions, but not in the setup and
> > teardown module functions.
> 
> No, that's not correct. aModule is the identical module you are in here. So
> why should those variables not be accessible? All of our tests would have
> been failed before we made use of it.

So then as a style guide, should we use:

1)
> function setupModule(aModule) {
>   aModule.x = {}
>   x.y = {}
> }

or 2)
> function setupModule(aModule) {
>   aModule.x = {}
>   aModule.x.y = {}
> }

2) Is more clear, but also more verbose.
The aModule part is/was important if you include another module by the old importer code. The question is if we really want to do that in tests, or not given that we now have the require statement. Personally I would meanwhile say we should drop this functionality completely from Mozmill and only rely on require. But no work should be done here given that we want to transition to Marionette. 

So maybe we should revert those changes and remove aModule completely? It may be worth starting a discussion on the mailing list to get an agreement for it.
(Assignee)

Updated

a year ago
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.