Overhaul test implementation 👷🏗 #1314

novemberborn · 2017-03-20T15:22:27Z

Another massive PR… the first bunch of commits are cleanups and refactorings that lay groundwork for the subsequent work.

Highlights:

t.throws() and t.notThrows(), when passed a promise or observable, no longer reject the returned promise with the AsssertionError. Instead the promise is fulfilled with undefined. This behavior is consistent with when a function is passed, and with other assertions which also do not throw when they fail.
Concurrent, Sequence and Test have been refactored and are (hopefully) easier to understand.
Similarly test-worker and process-adapter are slightly easier to understand.
It's now a failure to add an assertion after a test finishes. That said, the failure is only shown if you used t.throws() or t.notThrows() assertions with a promise or observable. I'll open an issue when this PR lands discussing how to make the failure count in other scenarios.
It's now a failure for a test to finish without executing any assertions. A failWithoutAssertions option has been added to the package.json configuration. It defaults to true, users should set this to false to disable the behavior. This will break deployments with where custom assertions are used, or tests that don't use AVA's assertion library for other reasons. Use t.pass() to ensure at least 1 assertion is executed.
We can now detect when t.end() is never called in a test.cb() test, or if a returned promise never resolves, or a returned observable never completes. This means tests will no longer hang. This does not work if user code keeps the event loop busy by starting or connecting to a server or running long timers.

This also prepares for integrating avajs/babel-plugin-throws-helper#8.

Don't make the duration comparison so precise, since timers may fire early.

* Clarify responsibilities * Consistently import dependencies * Clarify significance of `exports.avaRequired` * Stop mimicking process with process-adapter. Reference it using `adapter` instead. Use the `process` global where applicable. Masking the process global with a mimicked object is unnecessarily confusing. * Remove superstitious delays in exiting workers The worker now only exits when told by the main process. This means the IPC channel must have drained before the main process can send the instruction. There's no need to wait before sending the message that teardown has completed. The AppVeyor workaround was introduced to solve Node.js 0.10 issues. We're no longer supporting that version. In theory, issues around flushing I/O exist regardless of whether AVA is running in AppVeyor. There is no clear way around this though, so let's assume it's not actually an issue.

Asynchronous `t.throws()` / `t.notThrows()` was the only case where internal AssertionErrors were leaked to user code. Return `undefined` instead.

Simplify Runner by passing options to TestCollection.

There's no need to collect all results. Tests emit them directly.

Test results are emitted directly, so Test#run() only needs to return a (promise for) a boolean, indicating whether the test passed. Make the same change in Concurrent and Sequence, and return a boolean if all runnables passed. Refactor tests to stop relying on Test instances returning their result.

* Do away with underscore prefixes. They were used inconsistently, and are generally not worth it * Keep `_test` prefixed in ExecutionContext, since that is exposed to calling code * Reorder methods * Assume all arguments are passed and are correct. They're already validated in `test-collection.js` *Pass metadata when instantiating Tests * Rewrite test finish logic. There's still a fair bit of interleaving due to the `test.cb()` API, but it's now a lot easier to understand. Due to the last change, tests with `t.end()` and only synchronous assertions end immediately. Previously they would end asynchronously due to a promise being in the completion chain. Similarly, returning a promise or observable for a `test.cb()` test immediately fails.

Though currently this error is likely to get lost unless there is a pending assertion or `test.cb()` is used.

novemberborn · 2017-03-20T17:46:13Z

t.throws() and t.notThrows(), when passed a promise or observable, no longer reject the returned promise with the AsssertionError. Instead the promise is fulfilled with undefined. This behavior is consistent with when a function is passed, and with other assertions which also do not throw when they fail.

Need to update the type definitions to take this into account.

novemberborn · 2017-03-20T17:54:03Z

t.throws() and t.notThrows(), when passed a promise or observable, no longer reject the returned promise with the AsssertionError. Instead the promise is fulfilled with undefined. This behavior is consistent with when a function is passed, and with other assertions which also do not throw when they fail.

Need to update the type definitions to take this into account.

That said, it would make this example quite annoying:

const err = await t.throws(promise)
t.is(err.message, 'foo')

Because if err can be undefined, this would have to be written as:

const err = await t.throws(promise)
if (err) {
  t.is(err.message, 'foo')
}

And yes the code will crash without that guard, but that error is ignored given the original assertion failure in t.throws(). How accurate do we want to be?

sindresorhus · 2017-03-20T18:19:40Z

t.throws() and t.notThrows(), when passed a promise or observable, no longer reject the returned promise with the AsssertionError. Instead the promise is fulfilled with undefined. This behavior is consistent with when a function is passed, and with other assertions which also do not throw when they fail.

I don't get it.

test(async t => {
	const err = await t.throws(Promise.reject(new Error('foo')));
	console.log(err.message);
});

The console.log here will never be reached as t.throws throws:

test(async t => {
	const err = await t.throws(Promise.resolve(new Error('foo')));
	console.log(err.message);
});

What will change?

novemberborn · 2017-03-20T19:11:06Z

What will change?

It'll no longer throw, just like t.throws(() => {}) doesn't throw, and t.true(false) doesn't throw.

sindresorhus · 2017-03-21T12:33:53Z

I see. I use the pattern of assigning t.throws to err a lot, so having to guard err would be inconvenient. Maybe we should hold on off this until we've done #1047, so users doesn't have to change their code twice?

sindresorhus · 2017-03-20T18:35:50Z

test/fixture/stalled-tests/callback.js

@@ -0,0 +1,6 @@
+'use strict';
+
+const test = require('../../..');


These fixtures are transpiled with Babel, so you can drop the 'use strict'; and use import.

Yea I can clean that up. I probably just copied these from elsewhere.

sindresorhus · 2017-03-21T12:46:46Z

lib/sequence.js


-			const result = this.tests[i].run();
+			for (let next = iterator.next(); !next.done; next = iterator.next()) {


Why not just a for-of loop instead of extracting the iterator?

Because of promises. The alternative is maintaining an index variable to resume the iteration. That's what iterators do all by themselves though.

sindresorhus · 2017-03-21T13:00:17Z

lib/test-collection.js

@@ -124,14 +125,14 @@ class TestCollection extends EventEmitter {
 			context = null;
 		}

-		return new Test(hook.metadata, title, hook.fn, context, this._emitTestResult);
+		return new Test(hook.metadata, title, hook.fn, false, context, this._emitTestResult);


Time to use an object for the arguments?

Yea I was having my doubts about this. On the one hand it's an internal API so hey, but on the other it's a lot of arguments…

sindresorhus · 2017-03-21T13:04:39Z

readme.md

@@ -673,6 +676,8 @@ You can use any assertion library instead of or in addition to the built-in one,

 This won't give you as nice an experience as you'd get with the [built-in assertions](#assertions) though, and you won't be able to use the [assertion planning](#assertion-planning) ([see #25](https://github.com/avajs/ava/issues/25)).

+You'll have to configure AVA to not fail tests if no assertions are executed, because AVA can't tell if custom assertions pass. Set the `failWithoutAssertions` option to `false` in AVA's [`package.json` configuration](#configuration).


This feature, while useful, is going to hit a lot of people. Any chance we could add support for assertions throwing AssertionError?

I know we have #1094, but that's more about integrating with t and t.plan(). I sometimes use assertions helper, and would like to use them in AVA without having to do anything extra.

This feature, while useful, is going to hit a lot of people. Any chance we could add support for assertions throwing AssertionError?

That's not the problem, actually. Any custom assertion that throws an error causes the test to fail, just because tests shouldn't throw errors.

The issue is when all custom assertions pass and no errors are thrown. The intent is for the test to pass, but we don't know that custom assertions were executed. So it'll fail. That's why I added this option.

IMHO the question is what the default behavior should be.

Could we maybe have it on by default, but disable it if the user imports any of the popular assertion libs? That way we can have a good default, but also support people using external assertion libs without them having to configure something.

I'm not sure how we can make that detection reliable and consistent across test files.

Me neither. Maybe not worth it. We can discuss in a new issue after this is merged. Let's go with on by default for now.

sindresorhus · 2017-03-21T13:12:37Z

lib/test.js

+				this.finishDueToInactivity = () => {
+					const err = returnedObservable ?
+						new Error('Observable returned by test never completed') :
+						new Error('Promise returned by test never resolved');


❤️ This commit is an amazing improvement! I remember trying to achieve something like this in AVA 0.0.2, but I could not get it working. Probably because beforeExit didn't exist then.

sindresorhus · 2017-03-21T13:15:16Z

Concurrent, Sequence and Test have been refactored and are (hopefully) easier to understand.

Significantly more readable, indeed.

sindresorhus · 2017-03-21T13:15:56Z

Mark, you're on 🔥!

novemberborn · 2017-03-21T14:12:09Z

I see. I use the pattern of assigning t.throws to err a lot, so having to guard err would be inconvenient

OK so with 0.18:

import test from 'ava'

test('sync', t => {
  const err = t.throws(() => {})
  console.error('After sync', err)
})

This results in:

❯ "$(npm bin)"/ava --verbose test.js

After sync null
  ✖ sync Missing expected exception..

  1 test failed [14:07:57]

  sync
  /private/var/folders/_6/p8qxp_3n62zg9081tvb0lcc80000gn/T/tmp.HrBeKHcUVs/test.js:4

   3: test('sync', t => {
   4:   const err = t.throws(() => {})
   5:   console.error('After sync', err)

  Missing expected exception..

Now async:

test('async', async t => {
  const err = await t.throws(Promise.resolve())
  console.error('After async', err)
})

❯ "$(npm bin)"/ava --verbose test.js

  ✖ async Missing expected exception..

  1 test failed [14:08:26]

  async

  Missing expected exception..

What happens is that the promise returned by t.throws() is rejected, so the AssertionError is thrown by await and the subsequent code never runs.

With this branch:

❯ "$(npm bin)"/ava --verbose test.js

After sync undefined
  ✖ sync
After async undefined
  ✖ async

  2 tests failed [14:10:04]

  sync
  /private/var/folders/_6/p8qxp_3n62zg9081tvb0lcc80000gn/T/tmp.HrBeKHcUVs/test.js:4

   3: test('sync', t => {
   4:   const err = t.throws(() => {})
   5:   console.error('After sync', err)




  async

(OK just now noticing that the output for async seems to be truncated, that seems like a bug.)

Now, both sync and async behave the same. Will subsequent code crash? If so, that's the same as the current behavior for async. Will an assertion fail? If so, that's the same as the current behavior for sync. Can the user tell? No, since we only show the first assertion.

novemberborn · 2017-03-21T14:21:23Z

(OK just now noticing that the output for async seems to be truncated, that seems like a bug.)

Pushed a commit to fix this.

sindresorhus · 2017-03-22T08:39:18Z

Got it. My problem with this change is that it breaks a popular pattern that we also document:

test('async', async t => {
  const err = await t.throws(Promise.resolve());
  t.is(err.message, 'foo');
})

Will result in:

[TypeError: Cannot read property 'message' of undefined]

Which is not very helpful for the user.

Do you remember why and when we stopped throwing on assertion failures? Seems like that would have resolved all of this, as no code would run after a failed assertion. What is even the benefit of assertions not throwing?

Another workaround would be to just return an empty object until we implement #1047. I just don't want to force the user to change their code twice.

novemberborn · 2017-03-22T12:21:14Z

Got it. My problem with this change is that it breaks a popular pattern that we also document:
test('async', async t => {
  const err = await t.throws(Promise.resolve());
  t.is(err.message, 'foo');
})
Will result in:
[TypeError: Cannot read property 'message' of undefined]
Which is not very helpful for the user.

Interesting! That's happening because we now only check the pending assertions for failures after we've checked the return value. I think that's a bug, even if t.throws() returned a rejected promise we'd be wrapping it with a new AssertionError, since the Test code assumes that no AssertionErrors leak out of the test.

Do you remember why and when we stopped throwing on assertion failures? Seems like that would have resolved all of this, as no code would run after a failed assertion.

I don't know. It's hard to tell, currently empower doesn't rethrow but I don't know if it did before or if we disabled that.

What is even the benefit of assertions not throwing?

If you have a callback test (or even when you wrap a legacy API in a promise), code may run in a separate call stack. If the assertion throws that might become an uncaught exception, completely crashing your test run.

Another workaround would be to just return an empty object until we implement #1047. I just don't want to force the user to change their code twice.

That's an option, though with the bug I described above it would mean the t.is(err.message, 'foo') assertion gets reported, not the t.throws() assertion failure. And once I fix that bug I think not throwing becomes harmless, since subsequent errors are not shown to the user.

(Unless of course they cause an uncaught exception in some odd test, but even returning an empty object could cause that.)

novemberborn · 2017-03-22T12:45:36Z

That's happening because we now only check the pending assertions for failures after we've checked the return value. I think that's a bug

I've pushed a fix:

❯ "$(npm bin)"/ava test.js

  1 failed

  async
  /private/var/folders/_6/p8qxp_3n62zg9081tvb0lcc80000gn/T/tmp.KKt3WiWUVJ/test.js:4

   3: test('async', async t => {
   4:   const err = await t.throws(Promise.resolve());
   5:   t.is(err.message, 'foo');

  Test.<anonymous> (test.js:4:23)
  step (test.js:9:191)
  Test.<anonymous> (test.js:9:99)
  Test.__dirname [as fn] (test.js:3:1)

sindresorhus · 2017-03-22T13:47:56Z

@novemberborn Amazing. I'm good with it now.

jamestalmage · 2017-03-22T16:42:48Z

No comments on this PR yet, just some background:

As far as I know, we have never allowed assertions failures to throw. This is a relic from tap/tape which exhibit the same behavior. The big difference for those libraries is that they show the status of every assertion in their output. Not throwing allows them to check the status of future tests. We only show our first assertion failure. This is a result of our parallel execution of tests, which would make it difficult to inline assertions.

In tap / tape, this works as a good test for your add function:

test('add', t => {
  t.is(add(2, 2), 4);
  t.is(add(2, 3), 5);
  t.is(add(3, 3), 6);
  // ....
});

In AVA, you would stop at the first failure, so it's better to use macros:

function macro(t, lhs, rhs, expected) {
  t.is(add(lhs, rhs), expected);
}

test(macro, 2, 2, 4);
test(macro, 2, 3, 5);
test(macro, 3, 3, 6);

This moves each assertion into separate tests, so a failure of one does not mask failures down the line.

tap offers some convenience here for very simple tests. You don't need to separate it out like you do in AVA to see every failure.

Right now, what we are doing does not make a lot of sense. We only display the first failed assertion, but we continue on after that assertion collecting data we will never display. We have discussed a flag to display every failed assertion in #261.

jamestalmage · 2017-03-22T16:49:17Z

lib/test-worker.js

+	const path = require('path');
+	const chalk = require('chalk');
+
+	const isForked = typeof process.send === 'function';


I am pretty sure moving this in here will break karma-ava. Anything that relies on the Node environment (vs the browser) should remain in process-adapter.

process-adapter is swapped out by karma-ava when compiling for the browser with an implementation that polyfills any missing functionality.

karma-ava is pretty experimental though. We don't have a stable API so IMHO it's fine if this breaks karma-ava.

If necessary this could live in its own file. It's odd in process-adapter. Heck it's a bit odd to have it in test-worker versus just main.

I am not so concerned about breaking karma-ava in the short term. But we need a strategy for where we will locate Node stuff that needs to be polyfilled for the browser. This is going to necessarily create some "oddness", as you will encounter the need to abstract out stuff you might prefer to inline throughout the codebase.

Sure, let's cross that bridge when we get there.

jamestalmage · 2017-03-22T17:07:47Z

test/fixture/long-running.js

@@ -2,6 +2,7 @@ import test from '../../';

 test.cb('slow', t => {
 	setTimeout(t.end, 5000);
+	t.pass();


We shouldn't force users to use t.pass in this scenario. You can pass t.end as an error first callback, so passing t.end really should count as performing an assertion.

Good point. Will push a fix imminently.

jamestalmage · 2017-03-22T23:06:42Z

lib/sequence.js

-				return this._results();
+		let activeRunnable;
+		const onBeforeExit = () => {
+			if (activeRunnable.finishDueToInactivity) {


Where is the finishDueToInactivity method defined?

In Test. See 3e3b213.

Rather than keeping an infinite timer open, waiting for `t.end()` to be called, fail the callback test if it is not ended when the event loop empties. Similarly, fail promise/observable returning tests if the promise hasn't fulfilled or the observable hasn't completed when the event loop empties. Note that user code can keep the event loop busy, e.g. if it's listening on a socket or starts long timers. Even after this change, async tests may hang if the event loop is kept busy.

If the assertion fails, the AssertionError no longer has access to the stack of when the assertion was called. Record it before entering the promise chain.

There are too many parameters.

If a pending assertion fails before the test implementation returns a rejected promise, the assertion failure should cause the test to fail, not the promise rejection.

novemberborn added 10 commits March 19, 2017 14:49

Anticipate timers firing early

c02536f

Don't make the duration comparison so precise, since timers may fire early.

Remove unused hook class

264c816

Always skip todo tests

24fb3ab

Don't leak internal AssertionErrors to user code

602d16f

Asynchronous `t.throws()` / `t.notThrows()` was the only case where internal AssertionErrors were leaked to user code. Return `undefined` instead.

Pass bail option to TestCollection constructor

702fd5d

Simplify Runner by passing options to TestCollection.

Simplify Concurrent and Sequence

c9c4598

There's no need to collect all results. Tests emit them directly.

Try to fail test if assertions are added after test finishes

a710eab

Though currently this error is likely to get lost unless there is a pending assertion or `test.cb()` is used.

novemberborn mentioned this pull request Mar 20, 2017

Better throws helper #1315

Merged

sindresorhus requested changes Mar 21, 2017

View reviewed changes

This was referenced Mar 21, 2017

Simplify t.throws() #1047

Closed

Make snapshots usable again #1317

Merged

sindresorhus approved these changes Mar 22, 2017

View reviewed changes

jamestalmage reviewed Mar 22, 2017

View reviewed changes

novemberborn added 5 commits March 23, 2017 15:00

Fail tests that finish without running assertions

120d2b2

Record stack for async throws assertions

5e0d220

If the assertion fails, the AssertionError no longer has access to the stack of when the assertion was called. Record it before entering the promise chain.

Pass options object to Test constructor

1fee62e

There are too many parameters.

Fail test due to pending assertion, not a returned promise rejection

950e71d

If a pending assertion fails before the test implementation returns a rejected promise, the assertion failure should cause the test to fail, not the promise rejection.

novemberborn force-pushed the test-overhaul branch from a0bbb19 to 950e71d Compare March 23, 2017 15:01

novemberborn merged commit 9fdf234 into master Mar 23, 2017

sindresorhus deleted the test-overhaul branch March 24, 2017 02:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul test implementation 👷🏗 #1314

Overhaul test implementation 👷🏗 #1314

novemberborn commented Mar 20, 2017

novemberborn commented Mar 20, 2017

novemberborn commented Mar 20, 2017

sindresorhus commented Mar 20, 2017

novemberborn commented Mar 20, 2017

sindresorhus commented Mar 21, 2017

sindresorhus Mar 20, 2017

novemberborn Mar 21, 2017

sindresorhus Mar 21, 2017

novemberborn Mar 21, 2017

sindresorhus Mar 21, 2017

novemberborn Mar 21, 2017

sindresorhus Mar 21, 2017

novemberborn Mar 21, 2017

sindresorhus Mar 22, 2017

novemberborn Mar 22, 2017

sindresorhus Mar 22, 2017

sindresorhus Mar 21, 2017

sindresorhus commented Mar 21, 2017

sindresorhus commented Mar 21, 2017

novemberborn commented Mar 21, 2017

novemberborn commented Mar 21, 2017

sindresorhus commented Mar 22, 2017

novemberborn commented Mar 22, 2017

novemberborn commented Mar 22, 2017

sindresorhus commented Mar 22, 2017

jamestalmage commented Mar 22, 2017

jamestalmage Mar 22, 2017

novemberborn Mar 22, 2017

jamestalmage Mar 22, 2017

novemberborn Mar 23, 2017

jamestalmage Mar 22, 2017

novemberborn Mar 22, 2017

jamestalmage Mar 22, 2017

novemberborn Mar 23, 2017

		@@ -0,0 +1,6 @@
		'use strict';

		const test = require('../../..');


		const result = this.tests[i].run();
		for (let next = iterator.next(); !next.done; next = iterator.next()) {

		@@ -673,6 +676,8 @@ You can use any assertion library instead of or in addition to the built-in one,

		This won't give you as nice an experience as you'd get with the [built-in assertions](#assertions) though, and you won't be able to use the [assertion planning](#assertion-planning) ([see #25](https://github.com/avajs/ava/issues/25)).

		You'll have to configure AVA to not fail tests if no assertions are executed, because AVA can't tell if custom assertions pass. Set the `failWithoutAssertions` option to `false` in AVA's [`package.json` configuration](#configuration).

Overhaul test implementation 👷🏗 #1314

Overhaul test implementation 👷🏗 #1314

Conversation

novemberborn commented Mar 20, 2017

novemberborn commented Mar 20, 2017

novemberborn commented Mar 20, 2017

sindresorhus commented Mar 20, 2017

novemberborn commented Mar 20, 2017

sindresorhus commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Mar 21, 2017

sindresorhus commented Mar 21, 2017

novemberborn commented Mar 21, 2017

novemberborn commented Mar 21, 2017

sindresorhus commented Mar 22, 2017

novemberborn commented Mar 22, 2017

novemberborn commented Mar 22, 2017

sindresorhus commented Mar 22, 2017

jamestalmage commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment