1025850 - setupModule and teardownModule leak variables declared on the aModule argument

Status: RESOLVED INVALID

(Testing Graveyard :: Mozmill, defect)

Description

[Andrei Eftimie]

Attachment: 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`.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+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.

Comment 2

[Andrei Eftimie]

(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.

Comment 3

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

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.