Core: Allow hooks via this.hooks in nested modules.

[rwjblue]

During review of the new Ember testing RFC (emberjs/rfcs#232) the required hooks argument has "raised some eyebrows".

---

To summarize that RFC proposal briefly, I am proposing that we leverage nested modules as the default and stop wrapping QUnit functionality with our own custom APIs.

Roughly, instead of moduleFor (which generates a QUnit.module with the proper beforeEach/afterEach callbacks), we leverage nested modules and provide simpler helper functions that setup the beforeEach/afterEach:

// before
moduleFor('service:foo');
test(....);

// after
module('service:foo', function(hooks) {
  setupTest(hooks);
});

---

Some of the concerns raised during that RFC center around the hook object:

• Teaching folks what hooks means is a bit more difficult because it does not represent the "test environment", but rather just the hooks.
• Passing only hooks to the helper functions proposed in the RFC means that if we ever need to thread more information through, we either have to use hooks as a transport or change our API to add more arguments.
• It seems somewhat impossible to communicate across multiple helpers (again without using hooks as a state/transport mechanism).

The concerns seemed reasonable, so I figured I'd kick off some discussion here (with a PR adding the functionality that I think would assuage the concerns).

---

Open questions:

• Should I allow testEnvironment.hooks to already be present (e.g. module('foo', { hooks: 'fishing' }, function(hooks) {}))?
• Do we need any additional test cases (e.g. multiple levels of nesting, interchanging this.hooks.* with hooks.*, etc)?

[trentmwillis]

Pardon the long answer, I wanted to give a thorough response.

In my opinion, the "test environment" and "test hooks" are too intertwined in QUnit as it stands today. Consider the following:

QUnit.module('some module', {
  beforeRun() { }, // this is a user defined function that WILL be available while tests are running
  beforeEach() { }, // this is a test hook that is NOT available while tests are running
});

QUnit.test('test', function(assert) {
  this.beforeRun(); // Ok!
  this.beforeEach(); // ERROR!
});

This is unintuitive and makes it non-obvious to new users that you can set values for the test environment via the hash that is passed into QUnit.module.

So, it is my opinion that adding this.hooks to the nested module context will only further this confusion. Instead, I actually believe that the testEnvironment should not even be applied when we execute nested modules.

Instead, 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).

QUnit.module('some module', function() {
  this.beforeEach(someFunction);
  this.setTestContextProperties({
    beforeRun() {}
  });

  this.test('some test', function(assert) {
    this.beforeRun();
  });

  this.module('nested module', function() { /* ... */ });
});

So what does that mean for today? I do not think we should add testEnvironment.hooks as it will only further muddy the waters on what is available where. Instead, I would like to see us move towards an API like what I outline above.

We can do this by changing the documentation to no longer call the nested module argument hooks and instead change it to context (or something similar) and exposing new methods for defining properties.

Does this seem reasonable? I believe it addresses at least two of the concerns outlined above. The only one it potentially doesn't address is teachability. But, as I mention above, I actually think we need two distinct parts of the system here instead of them being intertwined.

Edit: A related issue that may be worth referencing: #923

[rwjblue]

Instead, we should have a clear separation between what I consider the "module context" and "test context (or environment)".
...
I actually think we need two distinct parts of the system here instead of them being intertwined.

I think this is absolutely great!

A related issue that may be worth referencing: #923

Sounds good, I'll migrate conversation over to that issue to iron out the specific details of the changes.

[trentmwillis]

Closing this as we will be pursuing the approach in #923.