Add module-contextual 'test' function

[gibson042]

trafficstars

(this is a restatement of https://github.com/jquery/qunit/pull/670#discussion_r26321413 , which itself referenced a discussion from https://github.com/jquery/qunit/pull/670#issuecomment-78513676 )

7be1d6c38cac4370bafdb17b10f0f4dee576bd12 introduced the QUnit.module( name, callback ) signature, which immediately invokes a callback to allow for definition of nested modules. However, doing so requires using global QUnit.module and/or QUnit.test functions: https://jsfiddle.net/k9af94x7/

This is ugly, and bad for the same reasons as use of global assertions like QUnit.equal instead of contextual assertions like assert.equal. We're fixing the assertions issue, and should do the same with module and test by making them accessible from either context or arguments of the module callback, while fulfilling some goals to the maximum possible extent:

• Intuitively-readable invocations, possibly paradigm-dependent (e.g., use of BDD names)
• Maximum similarity between module and test callbacks, for future iterative convergence
• Separation of concerns (e.g., no properties on assert that do not pertain to assertions, since we might want to allow integrating external assertion libraries)

My ideal vision would fully integrate the two functions, allowing code like:

// Note the second parameter
QUnit.test("grandparent", (assert, test) => {
    // `test` is destructurable, and contains helpers like beforeEach.
    // Those that are functions have signatures analogous to `test` itself.
    // Also present is environment, so users never need deal with `this`.
    test.beforeEach((assert, test) => { test.environment.ready = true; });

    // Any test can contain assertions, even those that would traditionally be modules.
    // But only child tests interact with beforeEach/afterEach.
    assert.strictEqual(test.environment.ready, undefined);
    test("uncle", (assert, { environment }) => {
        assert.strictEqual(environment.ready, true);
    });

    // As noted in https://github.com/jquery/qunit/pull/670#issuecomment-78513676 ,
    // a significant current difference between `module` and `test` is their
    // (a)synchronicity.
    // We can make that explicit (names subject to bikeshedding, but I think
    // defaulting to async will make for the smoothest transition.
    test.immediate("parent", (assert, test) => {
        test("child", (assert, test) => {
            assert.strictEqual(test.environment.ready, true)
        });
    });
});

However, if the above is a bit much, then I suppose we can just add module and test properties to the argument passed to module callbacks:

QUnit.module( "module b", function( hooks ) {
  hooks.test( "a basic test example 2", function( assert ) {
    assert.ok( true, "this test is fine" );
  });

  hooks.module( "nested module b.1", function( hooks ) {
    hooks.test( "a basic test example 3", function( assert ) {
      assert.ok( true, "this test is fine" );
    });
  });
});

Note that module is already incompatible with test, because arguments passed to their respective callbacks differ (respectively, moduleFns—a.k.a. hooks—vs. assert).

[Krinkle]

Adding test to the hooks object sounds good to me. I suppose it would make sense at that point to start referring in docs and examples to the hooks object by a different name, such as t.

[Krinkle]

(@leobalter wrote at https://github.com/qunitjs/qunit/pull/979#issuecomment-209002028):

Hooking the test on the callback arguments is the easiest part, the complexity comes registering the children tests as part of their parents. That's the biggest challenge at #978, I assume.

Hm.. enforcing this lexically would be nice indeed. However I think adding it as-is would be fine first step as well. Given that the closures are executed immediately, there is very little room for that to not work as expected. And it's certainly no worse than today.

We took a similar "fake it until you make it" approach for the Assertion API. We could add this in QUnit 2.x as an easy thing to optionally adopt and consider a more strict/lexically sound version for QUnit 3.0.

[Krinkle]

OK. Let's explore some of the options we've gathered here.

1. Simply add "test" and "modue" to the hooks object. E.g. hooks.beforeEach(); hooks.test(); hooks.module(…)
2. Simply add "test" and "modue" to the hooks object, but recognise that it is no longer just about hooks by documenting it with a better suited name going forward, to shift the way we recommend its use. E.g. "q", like q.beforeEach(); q.test(); q.module(…);
3. Turn the hooks object into the test function-object, with hook methods. E.g. test.beforeEach(); test(); test.module(…)

👎 Status quo

QUnit.module('Thing', hooks => {
  hooks.beforeEach(() => {
    // …
  });

  QUnit.test('foo', assert => {
    assert.true(Thing.foo(1));
    assert.true(Thing.foo(2));
  });

  QUnit.test('bar', assert => {
    assert.false(Thing.bar(4));
    assert.false(Thing.bar(7));
  });

  QUnit.module('SubThing', hooks => {
    QUnit.test('quux', assert => {
      assert.true(SubThing.quux(-1));
      assert.false(SubThing.quux(-2));
    });
  });
});

👀 Option 1: Simply add to the hooks object

QUnit.module('Thing', hooks => {
  hooks.beforeEach(() => {
    // …
  });

  hooks.test('foo', assert => {
    assert.true(Thing.foo(1));
    assert.true(Thing.foo(2));
  });

  hooks.test('bar', assert => {
    assert.false(Thing.bar(4));
    assert.false(Thing.bar(7));
  });

  hooks.module('SubThing', hooks => {
    hooks.test('quux', assert => {
      assert.true(SubThing.quux(-1));
      assert.false(SubThing.quux(-2));
    });
  });
});

🚀 Option 2: Simply add to the hooks object, and recommend local name of q

QUnit.module('Thing', q => {
  q.beforeEach(() => {
    // …
  });

  q.test('foo', assert => {
    assert.true(Thing.foo(1));
    assert.true(Thing.foo(2));
  });

  q.test('bar', assert => {
    assert.false(Thing.bar(4));
    assert.false(Thing.bar(7));
  });

  q.module('SubThing', q => {
    q.test('quux', assert => {
      assert.true(SubThing.quux(-1));
      assert.false(SubThing.quux(-2));
    });
  });
});

🎉 Option 3: Turn hooks object into test function object

QUnit.module('Thing', test => {
  test.beforeEach(() => {
    // …
  });

  test('foo', assert => {
    assert.true(Thing.foo(1));
    assert.true(Thing.foo(2));
  });

  test('bar', assert => {
    assert.false(Thing.bar(4));
    assert.false(Thing.bar(7));
  });

  test.module('SubThing', test => {
    test('quux', assert => {
      assert.true(SubThing.quux(-1));
      assert.false(SubThing.quux(-2));
    });
  });
});

---

0. 👎 Something else and/or keep current global static methods (please comment).
1. 👀 Add "test" and "modue" to the hooks object, under its current name. E.g. hooks.beforeEach(); hooks.test(); hooks.module(…)
2. 🚀 Add "test" and "modue" to the hooks object, lead with q as local name in documentation. E.g. q.beforeEach(); q.test(); q.module(…);
3. 🎉 Turn the hooks object into a test function-object, with hook method properties. E.g. test.beforeEach(); test(); test.module(…)

[Krinkle]

Would love to hear your thoughts, concerns, proposals, or just one or more emoji votes to show your support!

/cc @qunitjs/community

[Turbo87]

I have to admit that I'm not sure I understand what the benefit of this API change would be. To me these proposals look more verbose and complicated than the status quo (at least if you do something like const { test, module } = QUnit). We should also consider the cost to the users of such a fundamental API change. It might be possible to write a codemod for it, but I'm wondering if there might be some edge cases that it might not be able to cover.

[smcclure15]

What's to become of the QUnit.test syntax for either "flat" (non-nested) modules, or if no modules are used at all? Not to scope creep, but is the proposed concept moving in a direction where all tests must be declared within a nested module context?

Maybe I'm having trouble distinguishing what is the ideal end state (and maybe not attainable given history) versus some intermediate proposals to wean off of the current usage.

[Krinkle]

[Turbo87] I have to admit that I'm not sure I understand what the benefit of this API change would be. To me these proposals look more verbose and complicated than the status quo (at least if you do something like const { test, module } = QUnit).

For pure brevity, those static and global shortcuts indeed suffice. The intended benefit is to have known lexical bonds between a test and the module to which it belongs. This is not possible through global functions, I believe. Essentially it means that if code arrives later or is delayed by an await statement, the test or module might end up under an unrelated later module – whichever is the "current" one at that time that it executes.

[Turbo87] We should also consider the cost to the users of such a fundamental API change. […]

@Turbo87 Agreed. This would involve no breaking changes. It is entirely optional. For adopting the slightly different syntax, one would automatically benefit the added layer of confidence and strictness, but I don't think it warrants a breaking change or other mandatory/encouraged change to existing tests.

[smcclure15] What's to become of the QUnit.test syntax […]

It would remain, as does QUnit.assert, working exactly as today where the "current" module is found at runtime.

[smcclure15] what is the ideal end state

From a high level, I'd like to see an end-state where:

• … race conditions in complex user code that defines modules and tests produces an early, actionable. and easy-to-understand error message.
• … boilerplate like const { test, module } is not needed.
• … most developers find the syntax to feel familar, brief, and yet have confidence in correct and unambigious intent.
• … for existing code to reap the same guruantees as we offer today, with zero required changes.
• … for the improved version to look and feel the same as today, with little or no required changes.

From a technical level, with the currently presented ideas, it could work as follows:

• We expand hooks to gain test and module methods that naturally set their parent relations.
• If the new lexical form is used, but the parent module already finished, we emit an error.
• The static variants continue to exist and behave like today, where the parent module is found at run-time via config.current.

[raycohen]

What about (hooks, test, module) => which maintains the meaning of "hooks". Invocations like test.module() and q.beforeEach() make me feel like clarity is being lost

[Krinkle]

@raycohen I see what you mean with test.module() being a confusing mixture of APIs. It's not the option I would prefer, but I wanted to see how others felt about this as it was the only option offering both a single parameter and a bare identifier for test(), which some might like.

Regarding q.beforeEach(), the idea is that q is a local instance of QUnit. Where QUnit.test and QUnit.module are the global endpoints, q.test() and q.module() would be the nested calls to the equivalent interface. This is not unlike tape, which calls these tape and t. Although one could of course call the top-level one q as well if you like (e.g. const q = require("qunit");).

The q object would have capabilities that the global object doesn't, such as hooks. However, I don't see that as an issue per-se. If anything, it reminds us of the use case for "global" hooks, discussed at https://github.com/qunitjs/qunit/issues/1475, which we could naturally implement as QUnit.beforeEach().