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

Prevent incorrect use of t.throws() #75

Closed
sindresorhus opened this issue Apr 8, 2016 · 11 comments
Closed

Prevent incorrect use of t.throws() #75

sindresorhus opened this issue Apr 8, 2016 · 11 comments

Comments

@sindresorhus
Copy link
Member

I've seen a lot of incorrect use of the throws assertion in other test runner and even done the mistake myself sometimes. Now I'm beginning to see it with AVA too, so would be nice to be preemptive about it.

People don't realize they need to wrap their call in a function, so many end up doing t.throws(foo()) instead of t.throws(() => foo());. It's an easy mistake to make.

Happened yesterday: avajs/ava#739

We should disallow doing t.throws(foo()) and t.notThrows(foo()).

I think we can easily add auto-fixing to this by just wrapping the call in an arrow function like so t.throws(() => foo());.

@jamestalmage
Copy link
Contributor

Unfortunately this won't work.

foo() might return a promise, in which case, t.throws(foo()) is perfectly acceptable.

@jamestalmage
Copy link
Contributor

We could improve things with a custom babel plugin though:

  t.throws(foo());

becomes:

t.throws(throwsHelper(
  function () {
    return foo(a, b);
  }, {
    line: 3,
    column: 3,
    source: 'foo(a, b)'
  }
);

function throwsHelper(fn, data) {
  try {
     return fn();
  } catch (e) {
     throw new IncorrectThrowsError(e, data);
  }
}

Basically, we take the expression and wrap it in a function. Pass that function to the helper. The helper calls the wrapper function, returning the result. If it synchronously throws an Error - we print a good error message (including the source). Suggesting they wrap it correctly.

@jfmengels
Copy link
Contributor

That wouldn't work for nested functions, would it?

function foo(value) {
  return function() {
    // ...
  };
}

t.throws(foo(2)); // valid and proper use
t.throws(foo); // valid but not the expected use

You could force the user to give a non-call expression to throws

const fn = foo(2);
t.throws(fn);

But that sounds error-prone too and feels like just moving the problem somewhere else.

@twada
Copy link
Collaborator

twada commented Apr 8, 2016

For Promises, how about composing small helper function such as assert-exception with t.truthy

  • t.truthy(rejects(foo()))

Or introduce t.rejects ?

  • t.rejects(foo())

Then disallow doing t.throws(foo()) and t.notThrows(foo()).

@jamestalmage
Copy link
Contributor

That wouldn't work for nested functions, would it?

Yeah - it always would.

Take your examples.

t.throws(foo(2)); // valid and proper use

becomes:

t.throws(helper(function () {
  return foo(2);
}));

t.throws(foo); // valid but not the expected use

becomes:

t.throws(helper(function () {
  return foo;
}));

We are always just returning the evaluation of the expression. (i.e. we make no change to the value the assertion sees).

The only thing we protect against is a synchronous throw (and by protect, all I mean is we fail with a graceful error message that explains what they did wront).

@jfmengels
Copy link
Contributor

@jamestalmage Ah ok, you're just wrapping the call in an extra layer of try-catch 👍

@jamestalmage
Copy link
Contributor

@jamestalmage Ah ok, you're just wrapping the call in an extra layer of try-catch 👍

Yes. I decided to say that in the most verbose way possible!

@jfmengels
Copy link
Contributor

And you did well, now I got it ;)

@sindresorhus
Copy link
Member Author

@jamestalmage Using a Babel helper plugin is a really cool idea! Wonder if we can prevent/improve upon any other usage with Babel helpers.

For posterity:

@jfmengels
Copy link
Contributor

Should we close this issue? I have the impression that we can't do much statically.

@sindresorhus
Copy link
Member Author

Yup

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

No branches or pull requests

4 participants