Add beforeAll(), afterAll(), beforeEach(), afterEach()

[ryanblock]

Hello! I author and maintain many tape tests, and have found over the years that I frequently find a lot of setup, teardown, and repeat boilerplate code in these test suites. This could be greatly mitigated with the addition of optional beforeAll(), afterAll(), beforeEach(), afterEach() methods. (Happy to point to some examples if use cases are desired, although these kinds of features have become increasingly popular in other test libs.)

I'd propose that each of those methods run like any other test, but (as their names imply) be automatically inserted into the test flow before/after all tests in a given file (beforeAll(), afterAll()), and before/after each test in a given file (beforeEach(), afterEach()).

Unlike the module preloader and t.onFinish(), which are nice pre/post-run lifecycle hooks, I'd propose beforeAll(), afterAll(), beforeEach(), and afterEach() be incorporated into userland testing, and perform their own assertions, etc.

Example (pseudo):

test.beforeAll('Set up & test env')
test.afterAll('Tear down & test env')
test.beforeEach('Start service for each test')
test.afterEach('End service for each test')
test('Run a test')
test('Run another test')
test.onFinish('Run onFinish hook')
// Runs:
// - Set up & test env
// - Start service for each test
// - Run a test
// - End service for each test
// - Start service for each test
// - Run another test
// - End service for each test
// - Tear down & test env
// - Run onFinish hook

As alluded to in the example above, another reason I'd toss out in favor of adding this functionality is difficulty in maintaining large test suites where services or environments need to be set up and torn down at the beginning/end of the run. In that scenario, should one need to debug and isolate a single test, t.only() is not usable, because you still rely on the env setup/teardown tests; in such cases, I often find myself commenting out large blocks of code above and below the test I'm isolating, which is time consuming, error prone, and feels like a generally kind of janky workflow. (I assume I'm not completely alone in that, but who knows!)

Anyhow, happy to take a look at a PR with some guidance about if / how y'all would like to see it executed. Given that this is kind of out of band with how tape works today, would definitely want to get some feedback on how this would be best implemented.

[Raynos]

beforeAll is the first test statement in a file. afterAll is the last test statement in a file.

The use case of beforeEach & afterEach is better handled with a userland module like

• https://www.npmjs.com/package/redtape
• https://www.npmjs.com/package/tape-harness

Duplicate of #59

[Raynos]

You make a valid point that using the first & last test as beforeAll & afterAll does not play well with test.only(). Opening a seperate issue that adds an always option like

test('setup', { always: true }, (t) => {
  ...
})

test('one')
test('two')
test.only('three')

test('teardown', { always: true }, (t) => {

})

So that the setup & teardown always run even if test.only is called.

This property could be called something else like ignoreOnly.

[ryanblock]

I looked through closed issues and somehow didn't find #59, thanks for pointing me to that discussion.

An always option could def work instead of beforeAll() and beforeAll().

For the each methods, I think the issue still stands, and the use cases are worth evaluating. Per #59: yes, I can write a per-test setup/teardown function. In fact, I do that all the time. I find it messy and duplicative – it's more stuff to do and include with unrelated test logic. It's yet another thing to forget or incorrectly implement each time it needs to be there.

So I'm inclined to ask, given all that, why is adding additional dependencies necessarily better than adding support for beforeEach() + afterEach() as first class tape primitives?

[Raynos]

Adding new features to tape is harder then authoring a new dependency. We added async function support last year and that took a while to actually get everyone on board & released.

That being said I'm in favor of merging tape-harness into tape and exposing it as const createHarness = require('tape/harness') ( note it doesn't implement the beforeEach or afterEach API but achieves a similar result ).

[ryanblock]

Got it. Just to be clear, when you say "adding new features to tape is harder then authoring a new dependency", do you mean technically or politically (or both)?

Re. your tape-harness lib, I don't have much of an opinion about it as a solution specifically, but I'd generally advocate for the simplest, most straightforward possible API, which would be in keeping with tape's simple and excellent experience.

[ljharb]

beforeAll/afterAll hooks imo are a HUGE antipattern, and a massive source of bugs in many large test suites I've encountered. I would not be interested in supporting that pattern.

beforeEach/afterEach would be interesting, but it'd have to be able to run for a nested test too - eg:

var test = require('tape');

test('a', (t) => {
  t.test('b', (st) => {
  });
});

How would I attach beforeEach/afterEach behavior to the "a" test, and separately, to the "b" test?

[ryanblock]

Imo, I'd imagine beforeEach/afterEach running at "top-level" (non-nested) tests, but I can also imagine an option that lets you opt into nested tests. (Personally I don't use nested tests so I don't have a strong opinion on that.)

[ljharb]

Every feature on a top-level test imo should also work on a nested test; any place where that doesn't currently hold is an anomaly.

What would that look like, though? If it's an options object passed to each test call that contains a function, that could work, but how is that better than inlining the before/after steps, as @Raynos describes?

Also, what about:

var test = require('tape');

test('a', (t) => {
  t.test('b', (st) => {
  });

  t.test('c', (st) => {
  });
});

would a beforeEach for "a" run before b and c also? or only before a?

[ryanblock]

Every feature on a top-level test imo should also work on a nested test; any place where that doesn't currently hold is an anomaly.

sgtm

What would that look like, though?

Per above, I guess it doesn't have to look like anything. If you opt into beforeEach/afterEach, then it will just run literally before and after each. I'm not specifying that we must include options, just that if those are concerns, that's a possible available approach. I'm fine just being literal about it. Before/after each means after EACH.

So in your example:

beforeEach runs
test A runs
beforeEach runs
test B runs
afterEach runs
beforeEach runs
test C runs
afterEach runs
afterEach runs

If that's not the behavior one wants from nested tests, or they need more fine-grained control, one could simply not opt into this new feature. (Open to alternate approaches, just trying to speak to the questions!)

[ljharb]

Gotcha, that's certainly a reasonable answer.

It seems like the real value in this feature, then, only exists when using nested tests - if you don't have any, you can do test(msg, () => { beforeEach(); … afterEach(); }) with likely an identical amount of typing.

[Raynos]

do you mean technically or politically (or both)?

Last time I touched something related to how subtests work I broke someone else's workflow. Looking at the open issues, i suspect there's still unresolved sharp edges related to ordering edge cases.

My tape-harness library is sufficiently opinionated and works very well for my personal testing / tape workflows and doesn't break anyone else's code because it has 70 downloads instead of 500,000 downloads.

[ryanblock]

@ljharb yeah, although no typing at all would be ideal. Just declare test.beforeEach/test.afterEach once at the top and be done, unless I'm misunderstanding?

@Raynos fair enough, although I'd like to point out that unlike async, in this case I'm proposing as an additive change that someone would have to explicitly opt into, so it should be out of the way of existing code paths. ¯_(ツ)_/¯

[ljharb]

@ryanblock each test is an invocation, so mutating test wouldn't be able to work for nested tests - it'd have to be passed as an option to the call.

[ryanblock]

Ah! Well, diff use cases then I suppose! My use case, and I imagine the use case of the most folks, would be implementing for non-nested tests.

[ljharb]

I'm not sure about that claim; the majority of tape codebases I see (even the ones I don't maintain) use lots of nested tests.

However, if you don't have nested tests, then why not:

test(msg, (t) => {
  beforeEach();
  /* test code */
  afterEach();
});

? like what's the benefit of having a first-class feature for a top-level test that has no t.test calls inside it?

[cressie176]

I'm pondering exactly this problem with another test harness so thought I'd come and see how tape does things. My thoughts so far...

beforeEach / afterEach

if beforeEach / afterEach are implemented, they should work exactly as in the above snippet, i.e. as if they were function calls from within the test, and if the hook fails, the test fails. They should also be passed the t parameter, so that test can be programmatically skipped etc.

before / after

before / after add a surprising amount complexity, especially when nesting, but are useful for when global setup/teardown is slow and doesn't compromise test isolation. There doesn't seem to be support for these hooks in the TAP specification, or many other specifications that you might want to pipe TAP output to. Creating fake tests to report the status of the before/after hooks is an inventive, but surprising workaround, and one that might cause problems later on.

One potential alternative is to automatically fail tests following a before failure, but to leave their status untouched if after fails, so a passing test would stay passed. Providing the hooks are only used for managing test fixtures and not assertions, this is consistent with what actually happened, since the tests did actually pass. Instead, an after which fails could be reported with a TAP diagnostic line. This has the disadvantage that a test suite would not necessarily fail if the after hook failed, and may not be reported by piped reporters.

Another approach is be to bail out if either a before or after fails. This is supported by TAP specification and would give a clear indication that something went wrong, but could be annoying for those who prefer to continue running tests after error.

Will be interested to see how this issue progresses.

[mcollina]

Let me add a couple of cents to this discussion.

I would really like to see t.teardown(fn) added to tape.

One of the things I usually do is to create servers at the beginning of tests, and close them at the end of it. The usual pattern is to add a server.close() at the end of the test. However, that does not work well for failures, as it might never be reached.

[ljharb]

After using it in tap, I think that's the most appropriate. You can achieve "afterAll" semantics by wrapping everything in a test with a teardown, and "afterEach" by adding a teardown on each test.

A PR to add .teardown would be most appreciated.

[ryanblock]

Seeing this thread pop up again this morning, I realized I missed a question from you @ljharb!

? like what's the benefit of having a first-class feature for a top-level test that has no t.test calls inside it?

I would def expect the proposed feature(s) to have test calls! That's kind of the whole reason I oepend the thread, sorry if that's wasn't clear. :)

[ljharb]

@ryanblock so to confirm, would "run setup code before the tests in question" and t.teardown() solve your use case?

[Raynos]

A t.teardown(() => server.close()) would be really nice.

it would behave similarly to how defer is used in golang. Makes it a lot simpler to not forget to teardown that which is allocated.

[ljharb]

Here's my initial stab, which "works" but doesn't exactly match the expected ordering I wrote into the test: master...ljharb:teardown

Anyone who can get to it before I can is welcome to build on top of those tests.

[mcollina]

I did a slightly different implementation that delayed 'end' until all teardowns were run, and it also supported promises.

My fear in running after the current test is finished is to overlap with the next test.. which will make errors really hard to debug.

I'll push some code soon!

Your test is far more complete.0

[ljharb]

Absolutely i want teardown for one test to complete before the next test begins; promise support is super ideal (and will need more tests), and i have zero attachment to any particular solution at this point. I look forward to your implementation!

[mcollina]

@ljharb here is what I've done: https://github.com/substack/tape/compare/master...mcollina:teardown?expand=1.

Let me know if you want to send me a PR, it's very rough at this point.

[ljharb]

@mcollina i removed my implementation attempt, added Promise tests, and then cherry-picked your stuff on top (keeping one of your tests), and modulo one tiny modification to that test and to the implementation each, everything passes on both node 14 and 0.10 (the last pre-Promise node)! (i've updated my branch with that: master...ljharb:teardown)

Please do open up a PR (i can force push to it if needed); aside from // TODO we need an ending state and possibly more tests, this might already be good to go.

[ljharb]

teardown is released in v5.2.0. If there's still a need for further features in this vein, let's open up a new issue to discuss them :-)

[fregante]

It seems that a beforeAll was never accepted/implemented. In my situation I needed it to start an async service before the test run, but my current workaround is to move everything inside an async init function, including the tests:

async function init() {
	const service = await start();
	
	test("should ping back", async t => {
		t.equals(await service.ping(), "pong")
	});
}

init();

Not great, but it works. Ideally it would look like:

test.beforeAll(async t => {
	t.context.service = await start();
});

test("should ping back", async t => {
	t.equals(await t.context.ping(), "pong")
});

Prior art: https://github.com/avajs/ava/blob/main/docs/01-writing-tests.md#test-context

[ljharb]

@fregante your first example seems much better to me than a mutable t.context.

[fregante]

I don't particularly like wrapping test() calls in an init function, but there's also a context-less version:

let service;
test.beforeAll(async () => {
	service = await start();
});

test("should ping back", async t => {
	t.equals(await service.ping(), "pong")
});

Top-level await would help here, but it's not always available.

[ljharb]

In that case tho the beforeAll is acting just like an IIAFE.