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

Test: Allow async test defined from test callback parameters #866

Closed
wants to merge 1 commit into
from

Conversation

8 participants
@leobalter
Member

leobalter commented Sep 29, 2015

As requested on twitter by @fnando, this proposal brings a shortcut to define async tests through the test callback parameters.

This is based on the test callback function length and it is worth to investigate if it doesn't conflict anything on extensions like ember-qunit.

@fnando

This comment has been minimized.

Show comment
Hide comment
@fnando

fnando Sep 29, 2015

1MM ❤️ 😂

fnando commented Sep 29, 2015

1MM ❤️ 😂

@leobalter

This comment has been minimized.

Show comment
Hide comment
Member

leobalter commented Sep 29, 2015

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Sep 30, 2015

Contributor

Shouldn't cause issues with ember-qunit.

To allow folks to use this in ember-qunit I'll have to double check to confirm we pass the additional arguments through (and that the arity of the callback function is correct), but that shouldn't be considered a blocker for this...

Contributor

rwjblue commented Sep 30, 2015

Shouldn't cause issues with ember-qunit.

To allow folks to use this in ember-qunit I'll have to double check to confirm we pass the additional arguments through (and that the arity of the callback function is correct), but that shouldn't be considered a blocker for this...

leobalter added a commit to leobalter/api.qunitjs.com that referenced this pull request Sep 30, 2015

@@ -97,16 +97,20 @@ Test.prototype = {
QUnit.stop();
}
if ( this.callback.length > 1 ) {
done = this.assert.async.call( this.assert );

This comment has been minimized.

@mgol

mgol Sep 30, 2015

Member

Why not done = this.assert.async();?

@mgol

mgol Sep 30, 2015

Member

Why not done = this.assert.async();?

This comment has been minimized.

@leobalter

leobalter Sep 30, 2015

Member

sure! I was testing when I wrote this and I forgot to clean it up.

@leobalter

leobalter Sep 30, 2015

Member

sure! I was testing when I wrote this and I forgot to clean it up.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Sep 30, 2015

Member

It looks like a nice addition to make async testing quicker to set up. The implicit nature might also become a problem. What happens when some (private/custom) extension introduces that 2nd parameter already? Will the test time out eventually? Do we even have a default timeout?

Can we make the detection more precise? For example, make sure that the 2nd argument is actually called "done"?

Member

jzaefferer commented Sep 30, 2015

It looks like a nice addition to make async testing quicker to set up. The implicit nature might also become a problem. What happens when some (private/custom) extension introduces that 2nd parameter already? Will the test time out eventually? Do we even have a default timeout?

Can we make the detection more precise? For example, make sure that the 2nd argument is actually called "done"?

@fnando

This comment has been minimized.

Show comment
Hide comment
@fnando

fnando Sep 30, 2015

Can we make the detection more precise? For example, make sure that the 2nd argument is actually called "done"?

@jzaefferer I've been in projects that used done and next for this. Not sure if relying on the argument name is a good idea. But I think it is possible with func.toString() (works in node, not sure about browsers).

fnando commented Sep 30, 2015

Can we make the detection more precise? For example, make sure that the 2nd argument is actually called "done"?

@jzaefferer I've been in projects that used done and next for this. Not sure if relying on the argument name is a good idea. But I think it is possible with func.toString() (works in node, not sure about browsers).

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Sep 30, 2015

Contributor

make sure that the 2nd argument is actually called "done"

Relying on the argument name will roughly mean that testing minified (I often like to test minified production ready code to confirm just before a deployment) will fail since the minifier will have renamed the argument to whatever shortest symbol is available.

Contributor

rwjblue commented Sep 30, 2015

make sure that the 2nd argument is actually called "done"

Relying on the argument name will roughly mean that testing minified (I often like to test minified production ready code to confirm just before a deployment) will fail since the minifier will have renamed the argument to whatever shortest symbol is available.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Sep 30, 2015

Member

What happens when some (private/custom) extension introduces that 2nd parameter already?

That was my first concern and why I confirmed it is an ok for ember-qunit. I don't know about other QUnit usages that could brake with this option.

Member

leobalter commented Sep 30, 2015

What happens when some (private/custom) extension introduces that 2nd parameter already?

That was my first concern and why I confirmed it is an ok for ember-qunit. I don't know about other QUnit usages that could brake with this option.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Sep 30, 2015

Member

To be honest, I'm uncomfortable with automatically opening an asynchronous context by function arity. It makes future signature changes (including those that might further module/test convergence) awkward.

If we're comfortable with magic, though, maybe it could be slipped into asyncTest?

Member

gibson042 commented Sep 30, 2015

To be honest, I'm uncomfortable with automatically opening an asynchronous context by function arity. It makes future signature changes (including those that might further module/test convergence) awkward.

If we're comfortable with magic, though, maybe it could be slipped into asyncTest?

@Krinkle

This comment has been minimized.

Show comment
Hide comment
@Krinkle

Krinkle Sep 30, 2015

Member

The existing API is fine. I don't see why we need a way to do it in one line. It adds additional complexity, things that can go wrong, cause bugs etc., for a marginal benefit.

This looks like a case of brain/gut conflict where this will give the illusion of saving time because you may have to write less code. But that's a misconception. Developers rarely (if ever) spend the majority of their time writing the actual code.

I'm not principally opposed to having async-test-in-one-line, but I would recommend against this particular implementation. Per @gibson042, if we really want this, let's bring back asyncTest.

https://api.qunitjs.com/QUnit.asyncTest/

This method is deprecated and will be removed in QUnit 2.0. Use QUnit.test() in conjunction with assert.async() instead.

Member

Krinkle commented Sep 30, 2015

The existing API is fine. I don't see why we need a way to do it in one line. It adds additional complexity, things that can go wrong, cause bugs etc., for a marginal benefit.

This looks like a case of brain/gut conflict where this will give the illusion of saving time because you may have to write less code. But that's a misconception. Developers rarely (if ever) spend the majority of their time writing the actual code.

I'm not principally opposed to having async-test-in-one-line, but I would recommend against this particular implementation. Per @gibson042, if we really want this, let's bring back asyncTest.

https://api.qunitjs.com/QUnit.asyncTest/

This method is deprecated and will be removed in QUnit 2.0. Use QUnit.test() in conjunction with assert.async() instead.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Sep 30, 2015

Member

this example also solves the same issue, and it can be an external extension:

asyncTest = function( name, callback ) {
  return QUnit.test( name, function( assert ) {
    var done = assert.async();
    return callback.call( this, assert, done );
  });
};

the returns are there to keep the tests promiseable, and maybe the second one is not necessary.

Member

leobalter commented Sep 30, 2015

this example also solves the same issue, and it can be an external extension:

asyncTest = function( name, callback ) {
  return QUnit.test( name, function( assert ) {
    var done = assert.async();
    return callback.call( this, assert, done );
  });
};

the returns are there to keep the tests promiseable, and maybe the second one is not necessary.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 1, 2015

Member

Old:

asyncTest( name, function() {
  setTimeout( function() {
    start();
  } );
} );

New:

asyncTest( name, function( assert, start ) {
  setTimeout( function() {
    start();
  } );
} );

Certainly makes migration a lot easier, just add one argument (or two, if you're not using assert).

If that's what we go for, we should mention that on apsdehal/qunit-migrate#4

Could also use the opportunity to rename the method to testAsync, makes it easier to spot invalid usage by throwing an error from asyncTest.

Member

jzaefferer commented Oct 1, 2015

Old:

asyncTest( name, function() {
  setTimeout( function() {
    start();
  } );
} );

New:

asyncTest( name, function( assert, start ) {
  setTimeout( function() {
    start();
  } );
} );

Certainly makes migration a lot easier, just add one argument (or two, if you're not using assert).

If that's what we go for, we should mention that on apsdehal/qunit-migrate#4

Could also use the opportunity to rename the method to testAsync, makes it easier to spot invalid usage by throwing an error from asyncTest.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 1, 2015

Member

My example was referring to a custom asyncTest function, made externally. I prefer to leave asyncTest deprecated as it is and not keep a test method other than QUnit.test.

It makes future signature changes (including those that might further module/test convergence) awkward.

This is a good argument.


@fnando, as you suggested this, would bring some ideas and/or suggestions?

Member

leobalter commented Oct 1, 2015

My example was referring to a custom asyncTest function, made externally. I prefer to leave asyncTest deprecated as it is and not keep a test method other than QUnit.test.

It makes future signature changes (including those that might further module/test convergence) awkward.

This is a good argument.


@fnando, as you suggested this, would bring some ideas and/or suggestions?

@fnando

This comment has been minimized.

Show comment
Hide comment
@fnando

fnando Oct 1, 2015

@leobalter To be fair, using the arity as a way of determining if the test is async is not that magical and is a known technic from other libs (e.g. mocha.js).

I never really liked having two different functions like test and asyncTest, so I'd run away from that. If using arity is a problem, I would stick to assert.async(), even though I find it a boring task (small task, but a boring one).

tl;dr: I like this convenience, but I won't die if I can't have it. :)

fnando commented Oct 1, 2015

@leobalter To be fair, using the arity as a way of determining if the test is async is not that magical and is a known technic from other libs (e.g. mocha.js).

I never really liked having two different functions like test and asyncTest, so I'd run away from that. If using arity is a problem, I would stick to assert.async(), even though I find it a boring task (small task, but a boring one).

tl;dr: I like this convenience, but I won't die if I can't have it. :)

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 1, 2015

Member

@JamesMGreene, any addition?

Member

leobalter commented Oct 1, 2015

@JamesMGreene, any addition?

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 5, 2015

Member

I'm closing this. So far it seems better to wait for more discussion and arguments if we want to move this forward.

Member

leobalter commented Oct 5, 2015

I'm closing this. So far it seems better to wait for more discussion and arguments if we want to move this forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment