Krinkle
commented
1 week ago These many parameters, including boolean ones, don't read that well. But maybe it's not a big issue in practice since you'd normally see a condition here, not a direct boolean value.
Yeah, I'm not super happy with the complexity that this allows.
To me it is a trade-off for in favour of consistency and liberty. It makes sense to have test.if. And given the shape of the API, it would imho be expected and least surprising if the API looks the same as for any other test.* aliases like test.skip, test.todo, and test.only. Each of those has .each() so it makes sense to expose it here as well for consistency.
The downside is that if this freedom is misused, it can create some pretty complex tests. I've mitigated that by not explicitly demonstrating or encouraging this edge case in documentation. I think in most cases the way you reach this is naturally over time. When taken in small steps, I imagine developers would expect this to exist, which seems preferred over the surprise and cognitive complexity that would arise from us judging and omitting it in this one case. It remains of course, as always, the responsibility of developers and code review to judge their apetite for complexity, and what the alternative is.
The alternative in this case might be commenting out (reducing test coverage) or wrapping in an if-statement without leaving a trace of the skipped test.
Another alternative might be to use this extreme example as evidence that the API is too complex, and that this complexity even hides but exists in the simpler case of QUnit.test.if(name, cond, fn). If we believe that, we could move toward an API like assert.skip() where you'd have to write it like a normal test:
QUnit.test('example', function (assert) {
if (cond) {
assert.skip(); // throws special internal exception, stops test, marks it as skipped.
}
});I tend to dislike this because it's encouraging and normalizing use of assert in nested blocks. It also departs from the QUnit model where assertion methods are generally not test-ending. It also creates ambiguity over what to do with assertions that may happen before this (do we throw an Error to say that's invalid?). And whether to have beforeEach/afterEach. Even if there are no assertions, there may be other setup code in it, that would warrant before/afterEach. And in fact you may want to use some of your before/after code to power the conditional. That's all stuff we avoid with the declarative test.if model, at the cost of a longer method name, and longer signature.
This builds on previous ideas and discussions from https://github.com/qunitjs/qunit/issues/637, https://github.com/qunitjs/qunit/pull/434, and https://forum.jquery.com/portal/en/community/topic/conditionally-skip-tests.
Rationale
It is unacceptable to me for users to have to come up with constructs like the following:
Reading the pattern poses a barrier to new developers, because it doesn't look like a method call. We generally write conditional statements in source code as
if (true) { a(); } else { b(); }. You have to stare at it for a while, and realize that this was likely done to avoid having to duplicate the name and callback, since unlike in normal code, in tests it's kind of the point that name and callback are inlined for idiomatic and ergonomic reasons. This is the closest to preserving that, by utilizing "clever" tricks.Once a conditional branch is accepted in the top-level structure of a test suite, it invites treating the test suite as source code, allowing other complexity to creep in.
Writing a pattern like this also poses a barrier to people new to QUnit. There isn't anything like this in our documenation, and you'd have to feel confident and safe enough to employ tricks like this.
The pattern poses a barrier to static analysis (e.g. ESLint).
The existence of such tricks, to me, looks like a failure in our API design. It basically hints that we consider this use case invalid, and you had to take matters in your own hand. Looking at past discussions, it seems didn't care how ugly it looked, because I believed one would never need it if you did things "right". In reality, it seems inevitable that in a large codebase, you will end up having to skip tests in some browsers or some environments. Not because your code or the test is broken or "wrong" in some way, but because you're using progressive enhancement and not all make sense to run in all browsers.
This is especially likely when a project is isomorphic between Node.js and browser, or that supports a wide range of browsers, both of which are qualities that we very much cherish and encourage in the QUnit community. We should make this easy!
Implemenation
I originally wanted to call this
QUnit.test.skipIf()andQUnit.module.skipIf(), which would accept a "skip" condition, that executes the test if negative. I settled onQUnit.test.if() instead, accepting a "test" condition, which executes the test if positive. Three experiences led me to this:fixtures/skipIf.jsthe ones passedfalsewould have to execute.When migrating our own use cases within QUnit's own test cases, I repeatedly used it incorrectly, only to find the tests failing locally.
Virtually all our uses involve a positive value already, in the form of "supports X", to switch between "test" and "skip" respectively. I believe this idiom is more common than the notion of storing a negative condition to switching between "skip" and "test" (in that order).
After changing direction to
QUnit.test.if()and rewriting the source code, unit tests, internal adoption, and documentation, I realized I had written the documentation forskipIfincorrectly. After carefully adding a correct and working example, I wrote "if the condition is true it was equivalent to calling QUnit.test()". The opposite was true with the skipIf implementation.