Core: Introduce before/after hooks for modules #919

trentmwillis · 2016-01-12T20:04:24Z

As discussed in #893. Included tests for the following in support of this feature:

trentmwillis · 2016-01-15T22:22:38Z

@leobalter I think this is ready for review. Tried my best to keep the implementation within existing constructs, but required a few new properties to get all the features/behavior this should support.

leobalter · 2016-01-18T18:23:48Z

src/core.js

@@ -56,11 +58,14 @@ extend( QUnit, {
 			var module = {
 				name: moduleName,
 				parentModule: parentModule,
-				tests: []
+				tests: [],
+				testsRun: 0


this might be my only concern on this PR. Exposing a new detail on the API conflicts with the plans for the EventEmitter changes (#882).

Although, the tests array will still prevail and I wonder if we can set an equivalent property on the reference object for the test on that array. Doing that, we would have the tradeoff to filter the array, but it would save us a new API exposure.

Looking at the "events details types" issue, I think we would be able to replace module.testsRun with module.status.total when it is implemented since it looks like it will be similar.

I would also be fine with setting an equivalent property on the tests array, though it feels a little less correct.

Looking at the "events details types" issue, I think we would be able to replace module.testsRun with module.status.total when it is implemented since it looks like it will be similar.

👍 that would be great!

In the interim, should we proceed with this current implementation? (After I clean up the style issues, of course.)

Yes, sure.

We will need to have the API docs on http://github.com/jquery/api.qunitjs.com to be able to land it, would you mind to help on that part too?

I'll open a PR documenting the new before/after hooks for module. Is there anything else I should document?

I believe they'll be enough for now, QUnit docs will probably face a whole logging details review on a near future.

leobalter · 2016-01-18T18:32:04Z

The tests look great and I like the implementation.

My concerns remained only on the API as in #919 (comment)

Other noticed minor style issues for missing spaces between parenthesis, which are not blockers.

trentmwillis · 2016-01-19T21:16:47Z

Updated the PR (think I got all the style issues).

leobalter · 2016-01-19T21:23:10Z

src/test.js

+				test.preserveEnvironment = true;
+			}
+
+			if ( hookName === "after" && hookOwner.testsRun !== numberOfTests(hookOwner) - 1 ) {


a small style issue here numberOfTests(hookOwner) => numberOfTests( hookOwner )

Ah, missed one.

gibson042 · 2016-01-25T00:58:02Z

src/test.js

 			delete this.module.testEnvironment.beforeEach;
 			delete this.module.testEnvironment.afterEach;
+			delete this.module.testEnvironment.after;


This may require treating the change as breaking. @leobalter?

I don't think so. We have this delete statement only because we're introducing new properties to the modules' hook: before and after. Anyone previously extending that module with these keywords would get their tests affected.

That same could happen when we moved setup and teardown to beforeEach and afterEach. Similarly we could mention the introduction of QUnit.skip to the API that could interfere with any other use of custom made methods.

Anyone previously extending that module with these keywords would get their tests affected.

Exactly. Code like QUnit.module( name, { after: new Date(2016, 0, 1) }, … ), which is currently supported, will be broken. This is why I abhor the conflation of module options and test context in a single parameter, but there it is. And it's different from adding QUnit.skip because QUnit is our object, but that parameter is explicitly wide open for users, minus our exceptions.

Anyway, while maybe not a huge deal, this is technically breaking.

This is why I abhor the conflation of module options and test context in a single parameter

We should discuss a way to solve this before 2.0. Maybe we need to isolate the hooks from the environment or event remove the possibility to define env values on this object (let this do its job).

gibson042 · 2016-01-25T01:19:47Z

I wonder if this should cover edge cases like delaying the definition of a test. They're a bit pathological, but it would still be bad to run an after hook more than once.

trentmwillis · 2016-01-25T17:24:22Z

@gibson042 is that a real use case? If so, I don't really see a realistic way to handle it other than making an explicit declaration of the number of tests your module should have, since we need advanced knowledge of whether or not another test is expected to run.

leobalter · 2016-01-25T20:59:58Z

@trentmwillis: is that a real use case?

That example reflects tests added through async files loading, as in AMD. A dirty check could use a global variable to tell they are added.

Anyway, I also believe it's a developer responsibility to organize well tests introduced asynchronously. An update to the example - where I only added a single line: reveals a structure antipattern: https://jsfiddle.net/obuj0czg/1/

trentmwillis · 2016-01-28T03:54:50Z

Okay, makes sense. Should I attempt to cover that use case? Based on the example you gave, it probably doesn't make much sense without guaranteeing what module a test belongs to

leobalter · 2016-01-28T13:03:48Z

Should I attempt to cover that use case?

I believe a simple timeout example is enough to cover it up, testing it works properly on async operations and it does not leak anywhere else. AMD cases will be the responsibility for those who implements it.

gibson042 · 2016-01-28T14:57:13Z

Anyway, I also believe it's a developer responsibility to organize well tests introduced asynchronously. An update to the example - where I only added a single line: reveals a structure antipattern: https://jsfiddle.net/obuj0czg/1/

Right, pathological. 😄

gibson042 · 2016-01-28T15:04:21Z

Sorry about the noise. But my point was that we should add documentation and assertions that lock down how after behaves in such cases (either "invoked exactly once, and therefore ignored delayed tests" or "invoked whenever the module's test queue is empty, and therefore after delayed tests").

trentmwillis · 2016-01-28T16:06:43Z

"invoked whenever the module's test queue is empty, and therefore after delayed tests"

Sorry if I'm missing something obvious, but going back to my earlier point, if a test definition is delayed, then it may not get added to the module's test queue until after the module has already emptied it's test queue. How do we notify the module that even though it has ran all of it's tests, it should not run the after hook until more tests are added and ran?

gibson042 · 2016-01-28T16:24:24Z

How do we notify the module that even though it has ran all of it's tests, it should not run the after hook until more tests are added and ran?

We don't. We just document and assert our behavior, which will take one of two forms:

Invoke after exactly once. Tests added after its invocation will have to manage their own cleanup.
Invoke after whenever the module's test queue becomes empty. Tests added after its first invocation will result in successive invocations, and users employing such patterns must structure it to accommodate.

I'm just forcing the above as an explicit choice, rather than an accident of implementation.

trentmwillis · 2016-01-28T16:34:11Z

I'm just forcing the above as an explicit choice, rather than an accident of implementation.

Okay, cleared that all up. Currently the implementation means your second option is true (it will result in successive invocations), though I think the first option makes more sense. This is because I imagine after callbacks will often pair with before callbacks, which are only run once.

If that makes sense, I'll add a test for that here and document it in the documentation PR.

leobalter · 2016-01-28T16:50:24Z

If that makes sense, I'll add a test for that here and document it in the documentation PR.

@trentmwillis ++.

do you mind? It would be great to have it tested and documented.

trentmwillis · 2016-01-28T18:22:54Z

@leobalter updated with a test and updated the doc PR as well. Travis failed but it looks like it's for the same reason the master build is failing currently.

leobalter · 2016-01-28T18:41:40Z

I'm sick of browserstack-runner timeouts.

trentmwillis · 2016-02-01T16:49:01Z

@leobalter @gibson042 is the only remaining question whether or not we should consider this breaking? If so, I'd be fine with feature-flagging this somehow or delaying merge until 2.0.

leobalter · 2016-02-01T17:21:57Z

@trentmwillis, I really want to merge and release a new version with it, but I might need to release a major version.

I'm running on other necessary changes (the warning api) to release the next major, so releasing this feature won't be a problem.

trentmwillis · 2016-02-01T18:23:49Z

@leobalter a major version sounds good. Will let this sit until ready then.

stefanpenner · 2016-02-06T19:51:30Z

@trentmwillis good work!

trentmwillis · 2016-03-26T04:31:11Z

@leobalter this has been rebased and should be good to go. Care to do a once over since it's been awhile?

Closes qunitjsgh-919 Fixes qunitjsgh-893

jquerybot added the CLA: Valid label Jan 12, 2016

leobalter added the in progress label Jan 12, 2016

trentmwillis force-pushed the before-after branch 2 times, most recently from a46c1b9 to 64b93ea Compare January 15, 2016 22:05

leobalter reviewed Jan 18, 2016
View reviewed changes

trentmwillis force-pushed the before-after branch from 64b93ea to 9bdb29f Compare January 19, 2016 21:13

leobalter reviewed Jan 19, 2016
View reviewed changes

trentmwillis force-pushed the before-after branch from 9bdb29f to 6049d78 Compare January 19, 2016 21:55

trentmwillis changed the title ~~[WIP] Core: Introduce before/after hooks for modules~~ Core: Introduce before/after hooks for modules Jan 19, 2016

trentmwillis mentioned this pull request Jan 19, 2016

Entries: Document before and after module hooks qunitjs/api.qunitjs.com#118

Merged

gibson042 reviewed Jan 25, 2016
View reviewed changes

gibson042 closed this Jan 28, 2016

leobalter removed the in progress label Jan 28, 2016

gibson042 reopened this Jan 28, 2016

leobalter added the in progress label Jan 28, 2016

trentmwillis force-pushed the before-after branch 2 times, most recently from d4e3c5d to b4694d6 Compare January 28, 2016 18:02

trentmwillis mentioned this pull request Feb 1, 2016

Separate module options from test environment #923

Open

trentmwillis removed the in progress label Feb 22, 2016

trentmwillis mentioned this pull request Mar 2, 2016

moduleDone called before module is done #830

Closed

leobalter mentioned this pull request Mar 25, 2016

Next QUnit is 2.0 #975

Closed

leobalter added this to the v2.0 milestone Mar 25, 2016

Core: Introduce before/after hooks for modules

45874fa

trentmwillis force-pushed the before-after branch from b4694d6 to 45874fa Compare March 26, 2016 04:21

leobalter closed this in d452da1 Apr 18, 2016

flore77 pushed a commit to flore77/qunit that referenced this pull request Aug 10, 2016

Core: Introduce before/after hooks for modules

15775d2

Closes qunitjsgh-919 Fixes qunitjsgh-893

trentmwillis deleted the before-after branch March 2, 2018 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Introduce before/after hooks for modules #919

Core: Introduce before/after hooks for modules #919

trentmwillis commented Jan 12, 2016

trentmwillis commented Jan 15, 2016

leobalter Jan 18, 2016

trentmwillis Jan 18, 2016

leobalter Jan 18, 2016

trentmwillis Jan 19, 2016

leobalter Jan 19, 2016

trentmwillis Jan 19, 2016

leobalter Jan 19, 2016

leobalter commented Jan 18, 2016

trentmwillis commented Jan 19, 2016

leobalter Jan 19, 2016

trentmwillis Jan 19, 2016

gibson042 Jan 25, 2016

leobalter Jan 25, 2016

gibson042 Jan 28, 2016

leobalter Jan 28, 2016

gibson042 commented Jan 25, 2016

trentmwillis commented Jan 25, 2016

leobalter commented Jan 25, 2016

trentmwillis commented Jan 28, 2016

leobalter commented Jan 28, 2016

gibson042 commented Jan 28, 2016

gibson042 commented Jan 28, 2016

trentmwillis commented Jan 28, 2016

gibson042 commented Jan 28, 2016

trentmwillis commented Jan 28, 2016

leobalter commented Jan 28, 2016

trentmwillis commented Jan 28, 2016

leobalter commented Jan 28, 2016

trentmwillis commented Feb 1, 2016

leobalter commented Feb 1, 2016

trentmwillis commented Feb 1, 2016

stefanpenner commented Feb 6, 2016

trentmwillis commented Mar 26, 2016

Core: Introduce before/after hooks for modules #919

Core: Introduce before/after hooks for modules #919

Conversation

trentmwillis commented Jan 12, 2016

trentmwillis commented Jan 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leobalter commented Jan 18, 2016

trentmwillis commented Jan 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 commented Jan 25, 2016

trentmwillis commented Jan 25, 2016

leobalter commented Jan 25, 2016

trentmwillis commented Jan 28, 2016

leobalter commented Jan 28, 2016

gibson042 commented Jan 28, 2016

gibson042 commented Jan 28, 2016

trentmwillis commented Jan 28, 2016

gibson042 commented Jan 28, 2016

trentmwillis commented Jan 28, 2016

leobalter commented Jan 28, 2016

trentmwillis commented Jan 28, 2016

leobalter commented Jan 28, 2016

trentmwillis commented Feb 1, 2016

leobalter commented Feb 1, 2016

trentmwillis commented Feb 1, 2016

stefanpenner commented Feb 6, 2016

trentmwillis commented Mar 26, 2016