Separate module options from test environment

[gibson042]

Related:

• Allow testEnvironment to be accessed within a test without this #894 (comment)
• https://github.com/jquery/qunit/pull/919/files#r51184320
• Core: Nested modules #800 (comment)

Note that there are two ways to set before/after hooks (which are currently the only module options): as properties on an options object, and as methods on the argument passed to a callback. Exposing environment procedurally is easy (e.g., a writable environment property on the callback argument), but the declarative side makes backcompat tough. I'm not particularly thrilled with the thought of adding a fourth parameter, though... we could instead look for an environment property on the second argument, using it exclusively when present (technically backwards incompatible, but on the same scale as #919) and otherwise generating warnings whenever there's any non-before non-after property. All of which, of course, is assuming that keeping the declarative/procedural duality is valuable, about which I'm not fully persuaded but am inclined to keep for now (in part since the procedural interface is so new).

For example, all of these would generate the same environment for their tests:

QUnit.module(name, {
    beforeEach: function() {…},
    environment: { preserved: true },  // new
    ignored: true
});

// Define tests
…

QUnit.module(name, function( hooks ) {
    hooks.environment = { preserved: true };  // new
    hooks.beforeEach(function() {…});

    // Define tests
    …
});

QUnit.module(name, {
    beforeEach: function() {…},
    // For compat with current QUnit (if no environment is defined).
    preserved: true
});

// Define tests
…

But the last would issue warnings about deprecated options/environment mixing.

I'm not in love with this, but it's the best I've got. Other suggestions welcome.

[leobalter]

I'm not in love with this, but it's the best I've got.

<3

This is not easy to handle, but I can't bring anything better as well.

I'll add this as priority before the next major release (at least a ready PR)

cc @trentmwillis

[trentmwillis]

👍

This approach makes sense to me and if we land other breaking changes to the declarative hash (e.g., #919), then now seems like a good time to introduce this.

[rwjblue]

Moving conversation from #1200 here (as it seems directly related to this issue).

In #1200 (comment) @trentmwillis mentioned:

We should have a clear separation between what I consider the "module context" and "test context (or environment)".

The "module context" involves anything that is defined prior to a test running. This includes module hooks and any predefined values for the testing environment. I would also like to see it include module-aware methods for defining its tests and nested modules (see below) instead of having to rely on methods from the global QUnit object at all levels.

The "test context" thus involves the predefined values from the "module context" plus any additional values added while the test is running. The test context is also "reset" between tests (with the exception of modifications during the before hook).

I am happy to start working on this, but would like to confirm on specifics before getting started.

Here is my take away from this issue and the convo over in #1200 as it relates to nested modules:

• modify moduleFns to add test, module, setTestContextProperties functions
• deprecate the second argument completely (in favor of the "module context" functions: setTestContextProperties, before, beforeEach, after, and afterEach)
• modify this inside the callback so that accessing any of the testEnvironment properties issues a deprecation, and accessing new "module context" functions is possible
• documentation
  • split out usage of nested format from "classic" format
  • mention this.* methods being added in bullet points above
  • rename the argument for the callback from hooks to module

Does that path sound reasonable?

[trentmwillis]

modify moduleFns to add test, module, setTestContextProperties functions

This seems good to me. I'm not sold on the setTestContextProperties name, but that's a bikeshed. It should allow merging of multiple invocations though.

deprecate the second argument completely (in favor of the "module context" functions: setTestContextProperties, before, beforeEach, after, and afterEach)

I'm 👍 for this when using the nested form, but we'll need to keep it for the non-nested form. In the non-nested form we could add an environment property to set as mentioned above.

modify this inside the callback so that accessing any of the testEnvironment properties issues a deprecation, and accessing new "module context" functions is possible

Seems reasonable. Doing anything on this within the module callback other than call one of the provided hooks should basically do nothing.

[Krinkle]

Circling back to this. I'lll try to summarise the various links to and from this discussion and what changed in the meanwhile.

Current state (as of QUnit 2.10)

Every test has an environment object as its this object to use for things that may be put there by test runner plugins (such as Sinon sandbox methods) and for any state prepared by a before/beforeEach methods, which can then be torn down by after/afterEach.

This environment is created fresh for each test, and we also copy over any properties from the module object over to it as a start, which can be used for any shared fixtures etc.

Example:

QUnit.module('example', {
  abc: 123,
  beforeEach: function () {
    this.thing = new Letters(this.abc);
  }
});

QUnit.test('foo', function (assert) {
  assert.equal(this.abc, 123);
  assert.equal(this.thing.constructor, Letters);
});

At this point it's worth noting that properties like beforeEach on from the module object are passed on to module hooks, and then deleted from it, which means you can't do confusing stuff like see or call this.beforeEach() from inside a test.

The above is identical to the below which uses the nested modules API (#543, QUnit 1.12).

QUnit.module('example', (hooks) => {
  hooks.beforeEach(function () {
    this.abc = 123;
    this.thing = new Date(this.abc);
  });

  QUnit.test('foo', function (assert) {
    assert.equal(this.abc, 123);
    assert.equal(this.thing.constructor, Date);
  });
});

The above doesn't involve any literal module object, and doesn't make use of the fact that hook-named functions are consumed into the hooks and then removed. Note that abc is now defined inside beforeEach instead. If this were expensive, it could be done from hooks.before() instead. Or if the intention is that it is computed ahead of time or otherwise static/shared, it can be put in the lexical scope instead:

QUnit.module('example', (hooks) => {
  var abc = 123;
  hooks.beforeEach(function () {
    this.thing = new Date(abc);
  });

  QUnit.test('foo', function (assert) {
    assert.equal(abc, 123);
    assert.equal(this.thing.constructor, Date);
  });
});

Problem

I suppose the problem statement is that it is isn't obvious that the module() object literal is both a hook provider and used to create each test's environment object. I can also see it being surprising in edge cases that we detach the hook properties from it.

Proposal A

The original proposal by @gibson042 from 2016 (from the issue descrption):

• Add support for an environment property on the module options object. This would do the same as what the module options object curently does - which is, its properties are shared and thus copied over into the environment for each test.
• Add support for an assignable environment property on the hooks object for the same purpose.

Proposal B

I'd like to expose the possibility of deprecating and removing this behaviour rather than embracing it fully with a new module option. Per the examples in the status quo section, I think the use of such property is trivially replaced by either performing assignments from a before() hook, or by using a lexical variable.

Note that QUnit did not yet have a before() hook when we first started thinking about this (QUnit 2.0). At the time there was only setup (aka beforeEach).

As such, my proposal would be to deprecate this in a 2.x release (e.g. emit warnings for unknown keys in the module options object), and remove in 3.0.

[raycohen]

Am I reading your suggestion to mean that the following, which currently works, would not be valid in QUnit 3.0?

QUnit.module('example', function (hooks) {
  this.abc = 123;

  hooks.beforeEach(function () {
    this.thing = new Date(this.abc);
  });

  QUnit.test('foo', function (assert) {
    assert.equal(this.abc, 123);
    assert.equal(this.thing.constructor, Date);
  });
});

[Krinkle]

@raycohen No, your example would continue to work!

The proposal I wrote above would deprecate the ability to mix test hooks and test context in the module options object parameter:

// The below might be deprecated in QUnit 2.x and removed in QUnit 3.0

QUnit.module('example', {
  abc: 123,
  beforeEach: function () {
    this.thing = new Date(this.abc);
  })
});

QUnit.test('foo', function (assert) {
  assert.equal(this.abc, 123);
  assert.equal(this.thing.constructor, Date);
});

Your example, already separates the test environment and test hooks, so that's fine!

Note that it might be appealing to you to use JavaScript's own scope. This mean you no longer have to use the this object at all, which makes modern syntax easier to write as well, and means you have one less thing to learn and worry about:

// Classic syntax

QUnit.module('example', function (hooks) {
  var abc = 123;
  var thing;

  hooks.beforeEach(function () {
    thing = new Date(abc);
  });

  QUnit.test('foo', function (assert) {
    assert.equal(abc, 123);
    assert.equal(thing.constructor, Date);
  });
});

// Modern syntax

module('example', hooks => {
  const abc = 123;
  let thing;

  hooks.beforeEach(() => {
    thing = new Date(abc);
  });

  test('foo', assert => {
    assert.equal(abc, 123);
    assert.equal(thing.constructor, Date);
  });
});

The above is what I would personally recommend, however that's entirely optional. If you prefer to use the test environment "this" object, or are worried about having to change old code, then don't worry! I see no reason to deprecate this feature.

This issue is merely about how that feature is mixed up with the module options parameter.

I hope that helps!

[raycohen]

Thanks @Krinkle that is reassuring. I have found that lexical scoping can run into issues with nested modules depending on where you reassign, while using the this test environment is less easily be impacted by what happens in sibling modules. I might open another issue to discuss this since it's not really on topic with this thread.

[Krinkle]

@raycohen OK, I wouldn't mind discussing here as well. It sounds like you might have a use case where migrating to lexical scope would break or behave differently. If so, I might reconsider deprecating this. Could you elaborate on this with an example?

[raycohen]

when using nested modules to override variables from a parent module, using lexical scope doesn't always have the desired behavior. Many tests we write have the pattern where a top-level beforeEach does a bunch of shared work (some of it async) and then nested modules use the module context to override feature flags and other inputs to that shared work.

Here's a simplified example that I hope illustrates this - the test passes as intended when module context is used to override the top-level value but not when lexical scope is used:

http://jsfiddle.net/oL4d8g7z/

[Krinkle]

@raycohen Thanks, that makes more sense.

If I understand, the issue here is that the module scope function runs immediately since we first process modules recursively to register the tests, and then execute them in the second pass. This means that, when the first module executes for real, the later function was already executed.

The reason the "context" version is working is because QUnit made a copy of the this object, which is what the nested module is modifying. This is exactly the kind of "accident" I hope to make less likely to happen by recommending lexical approach, and hooks.

If this assignment was inside a hooks.before() call, the issue goes away and both pass: http://jsfiddle.net/ream736w/

The module function should generally not have any code inside of it, I think, unless used for dynamically generating test() cases.

A further modified version where both use the same approach consistently: http://jsfiddle.net/ream736w/1/

Having said that, I think you've convinced me that we should not hard-deprecate and remove for context variables in general. I would personally not recommend it at all, but I see not reason to discontinue supporting it. It's easy to keep working as it works today. If it already works for people, there is no reason to change that.

What I do think we should deprecate, is the ability to set variables on this via the options object parameter to QUnit.module() which is what this issue originally proposed. Do you currently use that?

[raycohen]

Ok, I see how hooks.before is accomplishing the same thing. I didn't have a good understanding of when before ran in nested modules.

One other concern I had with the lexical version is the possibility for "leaks" - i.e. that inserting/deleting/re-ordering nested modules that run before another nested module that does not itself set a lexical variable could impact it. Example: http://jsfiddle.net/v94pze5k/ - granted the developer will see this error at dev time (or via CI) but understanding why a leaky breakage like this happened in a large test file can be a lot of work.

As for the options object, we don't use that form, so I have no concerns with deprecating that behavior.