-
Notifications
You must be signed in to change notification settings - Fork 783
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
Make QUnit.config.beforeEach/afterEach functions rather than setters #665
Comments
P.S. I still also recommend that we move them to |
@jzaefferer @Krinkle @leobalter @scottgonzalez @gibson042 et al: Discussion needed here. |
The motivation for having a setter was to force global setup/teardown code into a single location.
That's an interesting example, though it reminds me of the arguments for API changes to accomodate qunit-composite. I would like to find solutions for these, but I don't think they should have a high priority over general API usage. In this particular case, a
I still don't find the given examples convincing. But then I've never used a test framework with nesting myself. The one time I helped debugging something in a nested testsuite involved Jasmine and CoffeeScript and was more an issue of implicit scoping in CoffeeScript... |
And I agree to keep it as it is for this reason. Nested suites can totally replace the need to have it with a different syntax. |
I think they're different arguments. The qunit-composite arguments were more of a nice-to-have/strawman upgrade, whereas the qunit-assert-step one is really a case of a custom assertion being forced to "do things wrong". However, I have since thought of at least one other workaround that actually feels more correct to me anyway: checking for |
@leobalter: Can you clarify this comment a bit?
I think it means that you're acknowledging that the future/potential nested suites feature may need to change this syntax/decision or add an additional mechanism. If that's at least acknowledged, I'm OK with closing this for now, though I can guarantee that nested suites will require just such a mechanism. 😕 |
IMO, with nested suites it won't be necessary to have the global before/afterEach hooks, so changing anything on them (global hooks) won't affect nested which seems to be the long and best sollution. The global hooks are interesting while we don't have the nested hooks implemented. |
Right, I was just hoping to kill 2 birds with 1 stone by consolidating the associated The way users choose to write their tests (linear modules vs. nested suites) would determine if the handlers they configure would operate on the global level or within suite contexts. |
So can we close this or not? |
I recommend reviewing the new PR #670 first as it leverages this concept. |
I did just did. That's definitely helpful, but I still don't think we can have that block 1.16 / 2.0. In that regard, turning around again, I'd rather not ship the global hooks now (remove them in master), instead of having to deprecate them soonish. I suspect that the effort of removing them and bringing them back later is rather small, since we can keep the infrastructure changes we made. |
@JamesMGreene @leobalter @Krinkle what do you think? |
I would recommend one of the following:
|
They should either be properties in |
We discussed this again during the meeting. Still have no consensus on the API. Will drop it for now to revisit it along with nested suites later. |
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
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
As I discussed with the rest of the QUnit core team in Chicago, I think that
QUnit.config.{beforeEach|afterEach}
should be changed to be functions rather than setters.This is beneficial in the following ways:
beforeEach
/afterEach
callback.QUnit.testStart
/QUnit.testDone
logging callbacks to achieve globalbeforeEach
/afterEach
behavior to get a proper extension point with access to the correct Test and Assert contexts rather than needing to rely onQUnit.config.current
. Clear evidence of this can be seen in the JamesMGreene/qunit-assert-step @ qunit-assert-step.js#L21-L26.v2.x
] if my forthcoming PR proposal for Issue Allow nested suites (modules) #543 (nested suites) is accepted.The text was updated successfully, but these errors were encountered: