Pass context as second parameter

[jamestalmage]

I originally proposed this back in #122 before I even knew about t.context. At the time, I closed it because t.context seemed sufficient. I am now starting to think it is a good idea again. I find myself want ing to shorten t.context.foo a lot. Right now that requires destructuring or a variable declaration at the top of each test.

// MUT = ModuleUnderTest
test.beforeEach(t => t.context = new MUT());

test(({context: mut, ...t}) => t.is(mut.foo(), 'foo');
// or 
test(t => {
  let {foo, bar} = t.context;
  t.is(foo(), 'foo');
});

If t.context were passed as a second parameter, both look better:

test((t, mut) => t.is(mut.foo(), 'foo'));
// or
test((t, {foo, bar}) => {
  t.is(foo(), 'foo');
});

Since t.context is the de facto way to pass around helpers that can't be shared across tests, it seems allowing more convenient access to its members is an easy win.

Is there any reason not to do this?

Can anyone conceive of a reason we will regret not saving that second parameter for something better down the road?

[jamestalmage]

FWIW, the implementation is super easy: #305

[vadimdemedes]

I'm not a fan of this to be honest. It does not really save any characters:

test(function (t, context) {
  context.key = 'value';
});

vs

test(function (t) {
  t.context.key = 'value';
});

The latter actually has less characters.

[sindresorhus]

Is there any reason not to do this?

We now have two different ways to do the same thing, which I'm not a super big fan of.

Would it have been less painful if we had named it t.ctx? Or is it that you can't use the helper names directly? Do you have an realistic example of when the verboseness is painful? I usually only use context for a few things, and the minor verboseness doesn't really bother me much.

[jamestalmage]

So, coming from mocha, I used to be able to do this:

var foo, spy;

beforeEach(() => {
  foo = new Foo();
  spy = sinon.spy();
});

it('foo.bar(a)', t => {
  foo.bar('a', spy);
  assert.spyCalledWith(spy, ...)
});

it('foo.bar(c)', t => {
  foo.bar('a', spy);
  assert.spyCalledWith(spy, ...)
});

Now I am left repeating a bunch of boilerplate.

The latter actually has less characters

Only because you called the variable context, if you called it c, you would have saved a few.

[vadimdemedes]

So where we are on this? In my opinion, shortening variable names (like c) is a bad practice, because only the original author can understand what those variables are for. It's only useful in JS1k competitions.

I'd stick with t.context or at least t.ctx, although "full" t.context looks simpler and better to me.

And passing context as a second argument results in 2 places where context can be accessed (what @sindresorhus said). Context is not a widely used feature, so we may use 2nd argument for something more common (e.g. maybe like done in mocha).

@jamestalmage there are workarounds for your issue/suggestion that don't involve modifying AVA:

1. var c = t.context at the beginning of the test
2. t.c = t.context in the beforeEach hook

[novemberborn]

Context is not a widely used feature

Anecdotally I rarely write a test that does not use context. Not sure where to find real-world data on that though.

OTOH it shouldn't be too hard to build a test interface on top of AVA which does expose context as a second parameter.

[jamestalmage]

I, like @novemberborn, use context a lot.

Here is a crazy idea. What if we used some sort of AST transform to turn this:

var x;

test.beforeEach(t => x = 'foo');

test('ends in "oo"', t => t.true(/oo$/.test(x)));

test('starts with "f"', t => t.true(/^f/.test(x)));

Into something that is async safe:

var x = {};

test.beforeEach(t => x[t.uniqueId] = 'foo');

test('ends in "oo"', t => t.true(/oo$/.test(x[t.uniqueId])));

test('starts with "f"', t => t.true(/^f/.test(x[t.uniqueId])));

[vadimdemedes]

It is, indeed, crazy :D

[jamestalmage]

It is, indeed, crazy :D

I know, I know.

[ariporad]

@jamestalmage: I think that an AST transform is likely to do more harm than good, from a debugging perspective.

I (personally) think that t.context is fine, especially if it's aliased it to t.ctx. It's not that long. But a note in the documentation that you can't use closures like you could in mocha would be good though, because it's not immediately obvious to newcomers.

[jamestalmage]

It's a pretty crazy idea, I agree.

The fact of the matter is that moving from mocha to ava is a paradigm shift that takes some getting used to.

I find that I'm using beforeEach less and less, and helper/init functions more. I still don't like how often my tests start with the same line:

test(t => {
  var foo = someInitFn();
  // ...
})

Seems like a DRY violation.

[ariporad]

@jamestalmage, @vdemedes, @sindresorhus: Here's an idea: what if we provided some way to pass arbitrary parameters to each test? Something like:

 test.beforeEach('setup foo', t => {
    const foo = makeFoo();
    return [foo];
});

// I'm unsure about passing params from prior hooks through, but you'll
// see why we need to below.
test.beforeEach((t, foo) => {
    // You can still do whatever with `t` here, including `t.context`
});

test.beforeEach(async (t, foo) => { // If a hook doesn't return something, then ignore it.
    return [await connectToDatabase()];
}); 

test((t, foo, db) => {
    // foo and db are a new for each test.
});

// If you think about it, we should probably pass these to `afterEach` hooks, because it's
// not unlikely that you'll want to pass a database or something else that needs to be
// cleaned up through.
test.afterEach(async (t, foo, db) => {
    await db.close();
});

This accomplishes the majority of you get with mocha and closures, while still being totally async safe.

While I think it's better to pass a bunch of parameters, this also opens up allowing people to pass context objects:

test.beforeEach(t => {
    return { foo: makeFoo() };
});

// This is why we need to pass params from prior hooks through
test.beforeEach('connect to db', async (t, ctx) => {
    return { db: await connectToDatabase(), ...ctx };
});

test(async (t, ctx) => {
    t.is(await ctx.foo.getDbData(ctx.db), [/* Whatever */]);
});

test.afterEach(async (t, { db }) => {
    await db.close();
});

Thoughts?

[vadimdemedes]

Hmm, what if I have multiple beforeEach hooks?

[ariporad]

@vdemedes: Take a look at my example, I added some more detail. Parameters from prior hooks get passed down to newer ones. If you want to have a context object, just do this:

test.beforeEach('setup foo', t => {
    return { foo: new Foo() };
});

test.beforeEach('connect to db', async (t, ctx) => {
    ctx.db = await connectToDB();
});

test((t, ctx) => {

});

The more I think about it, the more I like this way. It's very DRY, looks nice, is convenient, and I can't think of a use case where it wouldn't work. For people who really want a context object, we could even include a helper function inside AVA:

// ava/index.js
module.exports.ctx = function () {
    runner.test.beforeEach(function(t) {
        return t.context;
    });
};

[vadimdemedes]

Sorry, I don't think "waterfall context" would be a useful feature in AVA. Current context implementation is safe to use, because context is shared only between beforeEach/afterEach hooks (which run sequentially) and a single test. It means, that each test has its own context. If you need a global context, it'd be better to use a variable in the global scope, instead of passing it for each test.

Let me know if I understood something the wrong way. If you have a new idea, perhaps it's worth opening another issue, so that we don't mix things up here.

Originally, this issue was for providing a shorthand access to a context and I think we agreed, that having 2 different ways to do 1 thing would not be beneficial for us in the long term. I assume, this issue can be closed now?

[ariporad]

@vdemedes: No, I don't think you understand. In my idea it simulates waterfall context, but each thing is shared only between each beforeEach, test, and afterEach.

Example:

function Foo(arg) {
    this.arg = arg;
}

function Bar(arg) {
    this.arg = arg * 2;
}

test.beforeEach('make foo', t => {
    return [new Foo('this is a foo')];
});

test.beforeEach('make bar', t => {
    return [new Bar('this is a bar')];
});

test('test 1', (t, foo, bar) => {
    foo.bar = bar;
});

test('test 2', (t, foo, bar) => {
    console.log(foo.bar) // undefined
});

test.afterEach((t, foo, bar) => {
    console.log(foo.bar === bar) // Logs 'true' the first time, 'false', the second.
});

[vadimdemedes]

Oh, so return value from each hook adds to the arguments of the test function? Still, my opinion is the same. Also, I'm not a fan of dynamic function arguments. This would eliminate the possibility for us to provide something else in 2nd argument in the future.

[ariporad]

@vdemedes: sorry (I don't mean this argumentatively), but what was your opinion? The way I read your last comment it seemed like you didn't want non-async safe context, but this isn't that.

[jamestalmage]

@ariporad

Your idea requires reading through the entirety of the test code to understand what arguments are available and how to access them, remember that a beforeEach can technically be written after the current test you are reading, so you need to scan down as well. If you ever reorder your beforeEach tests, you then need to refactor any test that depends on that ordering. People who objected to the original idea will be even more against this one.

Going back to the original idea (context as a second arg), you can get at what you are pursuing using destructuring.

test.beforeEach(t => {
  test.context.foo = something();
  test.context.bar = somethingElse();
});

test((t, {foo, bar}) => {
  // now use foo and bar at will
});

Destructuring is also useful currently, but it's not very DRY

test(t => {
  let {foo, bar} = t.context;
});

[vadimdemedes]

@ariporad honestly, I just don't see what real-world benefits this would bring. As @jamestalmage pointed out, it would lead to mess in the tests. We already have context support, but this feels like a magical layer on top of it. And magic is hard to explain in the docs and maintain in the code. If you feel strongly about your idea, feel free to provide real project examples, where this feature would help. Maybe we could come up with another solution together ;)

[ariporad]

@vdemedes: Yeah, @jamestalmage explained why it wouldn't work. (Thanks, btw). I think the best thing to do is get rid of t.context, and pass ctx as a second param, and use destructuring.

[vadimdemedes]

@ariporad why getting rid of t.context would be a best thing?

[ariporad]

@vdemedes: because that way @sindresorhus won't get angry at us for having two ways to do something when we pass context as a second parameter. 😝

[vadimdemedes]

We now have two different ways to do the same thing, which I'm not a super big fan of.

I think we initially agreed that it probably won't happen, as we already provide context via t.context.

[ariporad]

@vdemedes: but I think it's worth considering what @jamestalmage was saying about being DRY. If you have context as a second parameter, then you can use destructuring to be rather dry. (Sorry, no example, on mobile)

[vadimdemedes]

The thing is, we don't want to pass context as a second argument. I think, we can use second argument for something more significant and widely used, t.end (done) for example.

To sum up, we have context support via t.context property. Offering a 2nd way to use it (do the same thing) is not optimal. @sindresorhus offered to rename t.context into t.ctx, but honestly I'd rather not do that either. Because ctx needs explanation, context does not. I'd rather keep things simple & clear, than short & weird.

[novemberborn]

The thing is, we don't want to pass context as a second argument.

Can we close this and #305 then? I don't have particularly strong feelings about this either way.

[sindresorhus]

Yes. It's more consistent and less magicky this way. With the current way it's clearer as you set the context the same way as you use it. With destructuring it's not really that much more verbose anyways. Readability and consistency over LOCs any day.