Interrupting test execution on first exception (through assertion extension API)

[jugglinmike]

As of version 0.21.0, Ava's built-in assertions do not influence the execution
of the test body. Specifically, when an assertion is violated, the test
continues to execute.

This behavior was previously reported as a bug (via gh-220). It was considered
"fixed" (via gh-259) with a patch that modified Ava's output.

This is a feature request for a behavior change: forcibly interrupt test
execution when an assertion fails via a runtime exception.

My use case is taking screen shots to assist debugging of integration test
failures. I have been able to react to failing tests programatically through a
combination of the afterEach and afterEach.always methods. When a test
fails, I would like to capture an image of the rendered application as this
can be very useful in identifying the cause of the failure (especially when the
tests run remotely on a continuous integration server).

Because the test body continues to execute following the failure, by the time
the afterEach.always method is invoked, the rendered output may no longer
reflect the state of the application at the moment of failure.

For unit tests, this might be addressed by making test bodies shorter and more
direct. Reducing tests to contain only one meaningful interaction would avoid
the effect described above. Because integration tests have high "set up" costs
and because they are typically structured to model complete usage scenarios,
this is not an appropriate solution for my use case.

Ava supports the usage of a general-purpose assertion library (e.g. as
Node.js's built-in assert module), and I've found that because these
libraries operate via JavaScript exceptions, they produce the intended results.
In the short-term, I am considering switching to one of these libraries.
However, Ava's built-in assertions have a number of advantages over generic
alternatives. In addition, restricting the use of Ava's API in my test suite
will be difficult moving forward--even with documentation in place,
contributors may not recognize that certain aspects of the API are considered
"off limits", especially considering that their usage does not directly effect
test correctness.

I haven't been able to think of a use case that would be broken by the change I
am requesting. But if there is such a use case, then this behavior could be
made "opt-in' via a command-line flag.

Thanks for your time, and thanks for the great framework!

[novemberborn]

Hi @jugglinmike, thanks for your suggestion. I'll respond more in depth later, but for now, for context, could you share how the integration tests happen within an AVA test()?

[jugglinmike]

Sure. Here's an example test for a "Todo list" application:

test.afterEach(t => { t.context._passed = true; });
test.afterEach.always(function(t) {
    if (t.context._passed) {
      return;
    }

    return t.context.driver.saveScreenshot(t.title);
  });

test('filtering', async (t) => {
    var driver = t.context.driver;

    await driver.create('first');
    await driver.create('second');
    await driver.create('twenty-third'); // intentional test bug
    await driver.complete(1);

    t.deepEqual(await driver.readItems(), ['first', 'second', 'third']);

    await driver.filter('Active');
    t.deepEqual(await driver.readItems(), ['first', 'third']);

    await driver.filter('Completed');
    t.deepEqual(await driver.readItems(), ['second']);
  });

(I've omitted details about creating the selenium-webdriver instance since
they do not seem relevant to the demonstration.)

The first assertion is violated due to an intentional test bug. Under Ava's
current behavior, the test continues to execute, driving the application
through more state changes. It ultimately completes after activating the
"Completed" filter. To be clear: the test is correctly reported as "failing."
The issue here is that the web browser under test is no longer in a state that
demonstrates the test failure, so the screenshot captured in the
afterEach.always hook is not useful.

By changing the three invocations of t.deepEqual to Node.js's
assert.deepEqual (for example), the first violation throws an exception, the
flow of the test is interrupted, and the afterEach.always hook is executed
while the browser is still in a state that is relevant to the failure.

[novemberborn]

Interesting. We actually have #261 which proposes that we report all subsequent assertion failures within a test, which is quite the opposite of what you're looking for.

Would it help if the --fail-fast flag made AVA blow up the specific test that failed with a thrown exception? The downside is that you'd only get one test failure in your CI run (#1158 notwithstanding).

[novemberborn]

Another approach might be to decorate test() and wrap the t object, assuming we expose the test state on it. That way you can build your desired behavior on top of AVA. For the time being I'd want to avoid adding an option to make these assertions throw.

[jugglinmike]

Would it help if the --fail-fast flag made AVA blow up the specific test
that failed with a thrown exception? The downside is that you'd only get one
test failure in your CI run (#1158 notwithstanding).

My goal is to give reviewers a clear picture of the entire problem. While this
serves that goal in one way, as you say, it detracts from it in another. I'm
not sure the trade-off would make that alternative preferable.

Another approach might be to decorate test() and wrap the t object,
assuming we expose the test state on it. That way you can build your desired
behavior on top of AVA.

I initially prototyped something like this, but I'm not comfortable maintaining
such a tight coupling with Ava's API. If Ava provided a more formal hook (e.g.
emitting an event for every assertion made), then I could implement this
functionality on my own without worrying about problems between major
releases... But that feature seems much more complex and specific to my use
case than simply throwing, so I wouldn't recommend it.

For the time being I'd want to avoid adding an option to make these
assertions throw.

I might be able to help more if I understood your reluctance. For the time
being, I'll use a dedicated assertion library and try to avoid this problem
informally by requesting that my teammates avoid the Ava-provided API.

[novemberborn]

@jugglinmike,

For the time being I'd want to avoid adding an option to make these
assertions throw.

I might be able to help more if I understood your reluctance.

Having the option will tempt people to enable it, even though they have no use for it. Indeed we want to run all assertions and provide an log that is even more complete than we have now.

I think I understand your use case and I'd like to support it, but I think this is better done in "user space". The question then is how AVA can provide the necessary hooks that you or another library author can build on top of.

ava-spec is a good example of such a library. We won't provide alternative test interfaces in AVA, but it's great that they're available regardless.

Another approach might be to decorate test() and wrap the t object,
assuming we expose the test state on it. That way you can build your desired
behavior on top of AVA.

I initially prototyped something like this, but I'm not comfortable maintaining
such a tight coupling with Ava's API. If Ava provided a more formal hook (e.g.
emitting an event for every assertion made), then I could implement this
functionality on my own without worrying about problems between major
releases... But that feature seems much more complex and specific to my use
case than simply throwing, so I wouldn't recommend it.

No this might work actually. E.g. t.on('assertionFailure', evt => evt.throw()). That could then throw from where the assertion was called, preventing your test from doing anything else. AVA would be expecting an exception so it would not report it.

You could either screenshot in the event handler or we could have another mechanism for an afterEach hook so you can officially tell the test result.

You wouldn't even have to wrap each assertion method, you'd just need to wrap the test implementations to intercept the t object.

[jugglinmike]

Having the option will tempt people to enable it, even though they have no
use for it. Indeed we want to run all assertions and provide an log that is
even more complete than we have now.

Due to that specific deficiency in Ava's logging implementation, I expect many
consumers are factoring their assertions to only share tests in cases where
the assertions are inter-related. Otherwise, in the event of failure, they will
only receive a partial view of the extent of the problem. So it may be that
consumers are already using the assertions as if they threw, and that by
extending the logs, Ava would only be giving them redundant information:
details about subsequent assertions that are already understood to be
interdependent. In that case, the logging fix would tend to motivate more
requests for "throwing" behavior.

But that's clearly conjecture on my part. I'm pointing it out just in case it
helps you weigh the relative merits of these solutions.

You wouldn't even have to wrap each assertion method, you'd just need to wrap
the test implementations to intercept the t object.

This satisfies my use case. Compared to wrapping assertion methods, it also
seems like a much more maintainable way to interface with this library. I would
still prefer the ability to make violation throw errors (and as mentioned,
other users might find that useful later on), but I certainly won't complain if
this solution is implemented. Sorry to say that I don't have the bandwidth to
help with that, though.

[novemberborn]

@jugglinmike yea, we'll have to see how things work out when we report multiple failures from the same test. Presumably we'd highlight the first and de-emphasize the others. The hope is that including them gives a fuller picture of why the test is failing.

I would still prefer the ability to make violation throw errors

Yea, we could add a sanctioned API so the event handler can perform the throw.

Sorry to say that I don't have the bandwidth to help with that, though.

No worries. Let's leave this issue open, if you or anybody else has the time then we can flesh out implementation details and make it happen.

[jugglinmike]

Sounds good to me!