Core: Skip tests with no/falsy callbacks

[gibson042]

I'm opening this as a pull request instead of an issue to make rejecting it easier if that is the final determination. If it is accepted, it should be coupled with a documentation change at jquery/api.qunitjs.com, which I am more than happy to submit.

In #637 (comment) and #434 (comment), @JamesMGreene advocated for a syntax like QUnit.test( "pending test" ). 3f08a1a then added QUnit.skip, but not the above Mocha-like implicit skip. Although I was not a fan of that style at the time, thinking about how to isolate Sizzle dependencies in the jQuery core suite has me reconsidering. In particular, I'd like to take advantage of something very similar in order to inline test preconditions rather than wrap tests in if blocks: QUnit.test( "name", preconditionsMet() && fnTest ).

Implementation broke no existing unit tests, and in fact I found an opportunity to use the new functionality within the logs tests as I was updating them: ae84fc3.

[leobalter]

765771c++

I don't like this kind of feature. Not passing a function parameter to the test seems too loose.

Depending on the kind of project, QUnit can be extended with @JamesMGreene's qunit-pending.

I repeat @Krinkle's arguments in #434 (comment)

My opinion on some of the mentioned use cases:

If you have tests for a features that doesn't exist yet (so an unconditional skip for tests that don't yet pass), the test should probably be in a feature branch instead, once the feature is implemented in the same branch, you merge it. No need to indiciate this within the test suite. As long as the tests are incomplete or complete but failing because the feature isn't there yet, that's good. Because if someone were to merge it too early, it's supposed to fail (not silently pass with a "skipped" message somewhere buried in the results). It also helps if your continuous integration environment supports testing branches, you'll be able to see which branches have work to do and work are ready to merge (at least from a test point of view).

If you need a reminder to write tests for something (so an unconditional skip for tests that don't exist yet), then file an issue in your issue tracker. Whether it is commented out or marked skip, I don't see how either is a workable solution to work as a reminder. Assuming the skipped tests should not affect the overal status of the suite as a whole, you'd only notice it if you look at the code or read through the results manually. Having an open issue (in a certain component, perhaps with label easy or test-todo) seems easier to find for (or give to) a new contributor, then having to run the tests and look for any skipped tests, which (given there are at least 3 reasons so far a test could be marked skipped) isn't a scalable workflow.

[gibson042]

If you have tests for a features that doesn't exist yet (so an unconditional skip for tests that don't yet pass), …

If you need a reminder to write tests for something (so an unconditional skip for tests that don't exist yet), …

Neither of those describe my use case, which is skipping tests at runtime for lack of client/library functionality:

• https://github.com/jquery/jquery/pull/1506/files#diff-1
• jquery/jquery@dd596cc#diff-0
• https://github.com/jquery/jquery/blob/f6302b0b53d61dfe1adbfaf6612be5cbced5bbc1/test/unit/css.js#L819
• https://github.com/jquery/jquery/blob/f6302b0b53d61dfe1adbfaf6612be5cbced5bbc1/test/unit/css.js#L756
• https://github.com/jquery/jquery/blob/f6302b0b53d61dfe1adbfaf6612be5cbced5bbc1/test/unit/selector.js#L264
• https://github.com/jquery/jquery/blob/f6302b0b53d61dfe1adbfaf6612be5cbced5bbc1/test/unit/support.js#L20
• …

[leobalter]

how about QUnit[ document.documentMode === 9 ? "test" : "skip" ]( ...?

This might also be abstracted in a function.

[gibson042]

Indeed. It's precisely because that expression is so damn ugly that I'm proposing this change, and will probably define conditionalTest( name, condition, fn ) or similar should it not go anywhere.

[leobalter]

@gibson042, would you make another PR for 765771c? That commit can be merged faster, I just want to assure we get a separate review for that, and maybe more eyes.

[gibson042]

#715

[jzaefferer]

Looks to me like you want to make the exceptions more explicit. If you're okay with using QUnit.skip, you could implement custom test methods that include the condition, e.g. testWithjQueryOffset() or testWithCreateHTMLDocument() or testInIE9().

For example (untested):

function testInIE9(name, callback) {
  QUnit[ document.documentMode === 9 ? "test" : "skip" ]( name, callback );
}

[gibson042]

We'll have enough separate conditions to make a single method more valuable. It may not be QUnit.test( name, falsy ), though; I just used that here because it was previously mentioned as compatible with other frameworks in the initial "skip" discussions.

[jzaefferer]

Let's revisit this after #715 landed, makes it easier to see the changes in the tests.

[leobalter]

Landed #715, @gibson042, would you rebase it? probably removing 765771c might solve it.

[gibson042]

Done.

[leobalter]

pull request instead of an issue to make rejecting it easier if that is the final determination

<3 <3 <3

Well, I'm feeling sad by still disagreeing with these proposed changes.

Maybe the other contributors might accept it, and I can say the changes are way easier to review now.

[jzaefferer]

I agree with @leobalter on this. @JamesMGreene @Krinkle do you want to take a look as well?

[Krinkle]

I agree with some of the previous comments. We should very much avoid patterns like QUnit[ bool ? "test" : "skip" ]( .., fn ). But I don't think it'd be an improvement if we add a trailing third argument to QUnit.test, or make the second argument toggle between boolean and function. That would swap one inline conditional for another, and isn't pretty either.

QUnit[ bool ? "test" : "skip" ]( name, function () {
} );

QUnit.test( name, function () {
 ..
 ..
 ..
}, bool );

QUnit.test( name, bool && function () {
});

Given the current API (QUnit.skip), I'd say its primary use case is to disable tests that are unstable or otherwise being worked on. Though, if necessary, a conditional skip can be done as follows:

QUnit.test( name, function () {
} );

QUnit.skip( name );

QUnit.skip( name, function () {
} );

if ( !bool ) {
  QUnit.skip( name );
} else {
  QUnit.test( name, function () {
  } );
}

The downside is that it repeats the test name twice and makes for a test suite where the top of your scope is no longer a clean uninterrupted flow of test definitions.

Note I generally discourage this kind of conditional skipping in test suites as, more often than not, it's indicative of a problem with the source code or its API. It should rarely be necessary to do that in the first place.

However, while it's slightly different than the API signature you propose, I'd implement "conditional skip" as additional argument in between the name and callback parameters:

QUnit.test( "name", bool, function ( ) {
 ..
} );

It avoids any dangling boolean expressions after the function, and avoids inline trickery with the callback parameter. The only thing I don't like about it is that it isn't clear what the boolean value is supposed to do.

It should be straight forward to implement, though. It changes the meaning of an existing parameter but does so in an unambiguous and backwards-compatible way.

Alternatively, I can also imagine having something like this:

QUnit.skipIf( bool, "name", function () {
 ..
} );

Thoughts?

[gnarf]

skipIf seems opposite of what the usecase wants to achieve, might be easier to write as:

QUnit.testIf( supported, "name", fn );

[jzaefferer]

So...?

QUnit.testIf = function( condition, name, callback ) {
  if ( condition ) {
    QUnit.test( name, callback );
  } else {
    QUnit.skip( name );
  }
};

Could just put that in a helper in Core somewhere, to see if that works.

[gibson042]

I guess the question is whether or not testIf belongs in core. Also, my crystal ball tells me that the next request is exposing a reason for skipping, so this might be best shelved for now.

[jzaefferer]

Well, I'm okay with that. We can also revisit this in the future, after all that's how we came up with skip.

[binodpanta]

Did we come to a conclusion on this? I am looking for a feature such as JUnit's assumeTrue where the test will not run if a condition is false

[platinumazure]

@binodpanta I don't know if this is something we want to reconsider for QUnit core, but this is something you can already do. You can just augment the QUnit object with a new method, as @jzaefferer demonstrated above.