Pass test's args to beforeEach/afterEach hooks

[tryzniak]

Now we can do stuff below:

test.beforeEach(async (t, args) => {
  if (args.initDb) {
    // initialise db
  }
});

test.afterEach(async (t, args) => {
  if (args.initDb) {
    // close db connection
  }
})

test('my test with args', async t => {

}, {
  initDb: true
});

I think this can simplify a lot of setup/teardown code flows.

Closes #2070

[novemberborn]

Nice! I've left some inline comments.

I think this needs to some change in the type definition as well. Unfortunately we won't be able to infer the argument types, but we should be able to type them as unknown[].

[tryzniak]

I've made some changes. Could not figure out where we should change typescript signatures though.
Other than that now we don't pass macros related args to beforeEach/afterEach hooks:

test.beforeEach(async (t, ...args) => {
  t.deepEqual(args, [{shouldLog: true}, {initDB: true}]);
}, {shouldLog: true});

function macro1(t, input, expected) {
  t.is(eval(input), expected);
}

macro1.title = (providedTitle = '', input, expected) => `${providedTitle} ${input} = ${expected}`.trim();

test(macro1, '2 + 2', 4, {initDB: true});
test(macro1, '2 * 3', 6, {initDB: true});
test('providedTitle', macro1, '3 * 3', 9, {initDB: true});

test('some test 1', async t => {
  t.is(1, 1);
}, {initDB: true});

[novemberborn]

Other than that now we don't pass macros related args to beforeEach/afterEach hooks:

Why?

Could not figure out where we should change typescript signatures though.

The definitions are here but you probably found them already. Looking at them now I think we're a bit stuck though. I think we should have a function that explicitly creates a macro, and change the other types to always forward extra arguments to the implementations. Regardless I don't see how we'd be able to type test arguments we'd now provide to the hooks. So tl;dr let's not worry about this for now.

[novemberborn]

Macros are not required to implement .title. They're not really distinguishable from regular test implementations.

[tryzniak]

I think I tried to nail down the very last argument of a test ({initDB: true} in the example) and only pass it to the hook, since stuff like '3 * 3', 9 won't be useful. Also document it and explain users that the last argument is always passed to the beforeEach/afterEach hooks.

test('providedTitle', macro1, '3 * 3', 9, {initDB: true});

But due to inability to really detect when something is a macro I'm puzzled for now.
And with my implementation args are and array, need to think about @Gwenio response.

[Gwenio]

I think I tried to nail down the very last argument of a test ({initDB: true} in the example) and only pass it to the hook, since stuff like '3 * 3', 9 won't be useful. Also document it and explain users that the last argument is always passed to the beforeEach/afterEach hooks.

test('providedTitle', macro1, '3 * 3', 9, {initDB: true});

But due to inability to really detect when something is a macro I'm puzzled for now.
And with my implementation args are and array, need to think about @Gwenio response.

If both hook args and macro args can both be of variable length then finding where to split them is not possible.
This means to support macros with rest or optional parameters that the hook arg count needs to be fixed.
Having them be fixed not only excludes a rest parameter, but also means the hook arg(s) cannot be optional.
And requiring a hook arg would be an interface breaking change.

To avoid this the hook arg would need to come before the test implementation(s).
Then it could be extracted when getting the optional base title and the implementation(s).
It would also need the type of the arg limited such that it is distinct from the implementation(s) (and the title, if it can come before that too).

The relevant part of the runner for extracting title and implementation(s) is line 60 to 65 of runner.js.
The hook arg should then be added to the task record when it is created on line 129.

[Gwenio]

I think this needs to some change in the type definition as well. Unfortunately we won't be able to infer the argument types, but we should be able to type them as unknown[].

If it was a single object as an argument, then it should be possible assigned a type similar to how the test context is currently.

So:

// example test file with typed hook argument
import anyTest, { TestInterface } from 'ava'

interface Context {
	/* whatever. */
}

interface HookOptions {
	/* whatever. */
}

const test = anyTest as TestInterface<Context, HookOptions>
// currently the above const would be:
// const test = anyTest as TestInterface<Context>
// for people upgrading from older versions, TSC will flag the need for a change with a compile error

test.beforeEach(async (t, options /* is type HookOptions */): Promise<void> => {
  /* hook body */
}

test('testing', async (t): Promise<void> => {
  /* test body */
}, {
  /* hook options */
})

test('another', async (t): Promise<void> => {
  /* test body */
}) // the hook options defaults to an empty object
// or TS has a compile error if HookOptions is not compatible with an empty object

In such a case if the basic untyped test is used, the hook options type would be:

// theoretical default hook options type
interface HookOptions {
  [key: string]: unknown
}

Doing the hook arguments this way would work fine for regular Javascript tests as well, they can use destructuring to get the options a particular hook cares about.

If the worry is about being more verbose, for test files with lots of hook options using an object will make setting up defaults easier.

const defaultHookOptions = {
/* most common values *.
}

test('test', async (t) => {/* test body */}, {
  ...defaultHookOptions,
  /* overrides specific to the test */
}

With a rest parameter, overriding the defaults for individual tests would be harder as it would be based on position.

In test files with few tests there is likely to also be few hook options. In files with many tests it is likely that for any particular option most tests will set it to the same value.

[novemberborn]

I'm struggling a little with the type definition implications. But we can ship this without improving those. I've just landed support for experimental features which we could use to make this an opt-in behavior until we figure out everything else.

There's this comment left as an outstanding issue though: #2223 (comment)

[novemberborn]

@tryzniak @Gwenio what do you think of adding these additional arguments to the t object instead? Wouldn't necessary make it easier to type but it may reduce some confusion when hooks are called with additional arguments.

[Gwenio]

@tryzniak @Gwenio what do you think of adding these additional arguments to the t object instead? Wouldn't necessary make it easier to type but it may reduce some confusion when hooks are called with additional arguments.

I made a fork with a branch demonstrating what I have in mind.

Not a finished set of changes, but should show what I am talking about.

[novemberborn]

Closing due to inactivity. We'd be happy to pick this up again when you have the time to get back to this.