Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support global test-level setup/teardown callbacks #633

Closed
jzaefferer opened this issue Aug 25, 2014 · 13 comments · Fixed by #635
Closed

Support global test-level setup/teardown callbacks #633

jzaefferer opened this issue Aug 25, 2014 · 13 comments · Fixed by #635
Assignees
Labels
Category: API Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors.

Comments

@jzaefferer
Copy link
Member

As originally discussed in #471, it is useful to have setup and teardown callbacks for each test that can be defined globally. For example, in ember the monkey-patching of QUnit.module could be replaced.

As an API I'm suggesting a simple property assignment. This ensures that there is only one place to do this (avoid scattering global setup/teardown across files) and it can be disabled (set to null):

QUnit.config.setup = function() {};
QUnit.config.teardown = function() {};

This would be simple to document as part of QUnit.config.

@JamesMGreene
Copy link
Member

It's a step in the right direction but I'd also like to see similar methods like "moduleSetup"/"moduleTeardown" as well (occurring once per module rather than once per test).

In the greater context, if we add per-module setup/teardown at the module level, we would either need to:

  • make the names something like setupOnce/teardownOnce if we want them to be similar to the current but still being backward-compatible, or
  • change the names completely (or offer aliases) to something like before/after or beforeAll/afterAll (per module) and beforeEach/afterEach (per test), a la Mocha.

@jzaefferer
Copy link
Member Author

"moduleSetup"/"moduleTeardown" (occurring once per module rather than once per test).

I reviewed this suggestion in the PR I closed and discussed it with Leo. The main reason why I think that this is a bad idea is the rather loose runtime behaviour of modules. The rerun-failed-tests-first ("rerun") feature of QUnit means that any test from any module can ran before other tests in the same module. From that perspective, a setup that runs only once per module makes no sense, since its impossible to reason about when that should run in the "rerun" case.

If that makes sense, the naming discussion shouldn't be necessary. Otherwise I'll get back to that as needed.

@JamesMGreene
Copy link
Member

Hmm, OK, I can definitely see the potential conflicts between the reorder config and a module-level setup/teardown. I still think it would be useful but I'm OK proceeding without it, at least for now.

So, moving on, my next question would be: is QUnit.config the right place for those function (vs. QUnit or QUnit.module (static))?

QUnit.config.setup = fn;
QUnit.setup = fn;
QUnit.module.setup = fn;

@jzaefferer
Copy link
Member Author

The advantage I see of using QUnit.config is that its already in use in projects that have enough customisations to justify a separate file for configuring QUnit, like the ember file I linked to above (which mirrors what I've been using in an app project with a big testsuite). For example, ember sets "QUnit.config.hidepassed" in that file, which is documented as part of QUnit.config, so finding QUnit.config.setup there would be easy enough.

Otherwise the three options you suggest are pretty much equivalent, I don't see any drawbacks to either of them.

@JamesMGreene
Copy link
Member

I'm not overly concerned about it, was just curious about the rationale. Honestly, regardless of where we put it among these 3 options, I'm not sure if a new user would be able to intuitively understand the purpose of methods named setup/teardown at those scopes without looking at the documentation.

e.g. some possible confusions:

  • "Set up the QUnit config object?"
  • "One-time setup of QUnit core?"
  • "Set up for every module?"

Correct interpretation: "Set up for every test"

So... maybe QUnit.test.setup? shrugs Not sure.

@jzaefferer
Copy link
Member Author

That is a good point. We're working on the implementation and struggle to find better names. How about picking something from other frameworks thats a lot more explicit, like "beforeEach" and "afterEach"? That'll be inconsitent with the setup/teardown of module, but we could rename those (with the usual deprecation process...).

@JamesMGreene
Copy link
Member

I personally really like the terminology of beforeEach/afterEach used in Mocha. And yes, we could do another 1.x release to deprecate Module#setup/Module#teardown to the new aliases of Module#beforeEach/Module#afterEach.

Another thing to keep in mind with all of this is the fairly closely related/integrated changes needed for #543.

@leobalter
Copy link
Member

before/after would look better, but I prefer going with beforeEach/afterEach to have a similar approach from Mocha.

@jzaefferer
Copy link
Member Author

Ignoring #543 for a moment, it looks like we agree on beforeEach/afterEach as properties of QUnit.config and on the module level. For the latter, we'll provide a compatibility layer that maps setup and teardown to the new names. @leobalter is implementing that in #635.

Once we have consensus on the nested modules, we can land this as-is or adapt accordingly.

@JamesMGreene
Copy link
Member

Sounds like a plan. 👍

@Krinkle
Copy link
Member

Krinkle commented Aug 26, 2014

Following pull #635.

Having global setup/teardown could be useful. But do we need to rename them from setup/teardown to beforeEach/afterEach? I'm not sure I see the value or justification in "renaming them at the same time". The module-specific setup/teardown hasn't changed in behaviour or signature, we'd be deprecating that for no reason other than the name.

Aside from the naming, there's nothing here that requires existing code to change, that's quite valuable from an API point of of view to maintain. What's the advantage of "beforeEach" and "afterEach"?

@jzaefferer
Copy link
Member Author

The only reason for the rename is consistency with the new methods we're adding, which also match the naming in other libraries. There's no other change. We considered using setup/teardown for both, but that is far from intuitive.

I think its better to fix the naming now, while we're migrating other APIs, then keeping the inconsistent naming around forever.

Btw., since you wrote "could be useful", here's another usecase for this ticket, in my second comment: qunitjs/api.qunitjs.com#63

@jzaefferer
Copy link
Member Author

@Krinkle does the comment above address your concerns?

leobalter added a commit to leobalter/qunit that referenced this issue Sep 1, 2014
leobalter added a commit to leobalter/qunit that referenced this issue Sep 1, 2014
jzaefferer added a commit to jzaefferer/qunit that referenced this issue Oct 10, 2014
Between locating the hooks in `QUnit` or `QUnit.config` and making them
simple setters and callback lists (like QUnit.done et al) and upcoming
plans for nested suites, we decided not to release this feature, for now.

I'm keeping the abstractions for hooks in place, so it should be trivial
to bring this back in whatever form we decide on later.

Effectively reverts 5ee31a0 and follow-up
commits.

Fixes qunitjs#665
Ref qunitjs#633
Ref qunitjs#635
Ref qunitjs#647
jzaefferer added a commit to jzaefferer/qunit that referenced this issue Oct 17, 2014
Between locating the hooks in `QUnit` or `QUnit.config` and making them
simple setters and callback lists (like QUnit.done et al) and upcoming
plans for nested suites, we decided not to release this feature, for now.

I'm keeping the abstractions for hooks in place, so it should be trivial
to bring this back in whatever form we decide on later.

Effectively reverts 5ee31a0 and follow-up
commits.

Fixes qunitjs#665
Ref qunitjs#633
Ref qunitjs#635
Ref qunitjs#647
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: API Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors.
Development

Successfully merging a pull request may close this issue.

4 participants