ava icon indicating copy to clipboard operation
ava copied to clipboard

Support nested groups?

Open novemberborn opened this issue 9 years ago • 87 comments

Currently the test files support a flat list of tests, each sharing all before , beforeEach, afterEach & after hooks. I find it useful to declare nested groups with their own hooks, while inheriting context from the parent.

For example I'm stubbing promise behavior and need to write test for if the promise is fulfilled or if it's rejected. Ideally I could do something like:

test.beforeEach(t => {
  t.context.behavior = sinon.stub({ getPromise () })
  // Return a pending promise
  t.context.behavior.getPromise.returns(new Promise(() => {}))
})

// Add optional title
test.group('promise is fulfilled', () => {
  test.beforeEach(t => {
    t.context.behavior.getPromise.returns(Promise.resolve(true))
  })

  test(async () => {
    const value = await t.context.behavior.getPromise()
    t.true(value)
  })
})

test.group('promise is rejected', () => {
  // You get the idea…
})

Each group would maintain its own list of hooks. Hooks could be inherited from the parent group at runtime or when initializing. There would be a root group for the test file itself.

This would allow building alternative test interfaces alongside AVA.

I'd be happy to try and do a PR for this if there's interest.


P.S. AVA looks really intriguing! Test runners could do with some shaking up :+1:

novemberborn avatar Nov 16 '15 15:11 novemberborn

I think it has some potential as a feature request. Something like describe in mocha. At the moment, we'd like to merge currently open PRs and do a complete cleanup/refactoring, and then continue with feature development.

If you'd be interested in making a PR, that'd be great. I don't recommend you to do that now, because after the rewrite, your PR will have a lot of merge conflicts.

@sindresorhus what do you think?

vadimdemedes avatar Nov 16 '15 15:11 vadimdemedes

From a quick skim this looks like the same as https://github.com/sindresorhus/ava/issues/62 just with a different name.

I'm not a big fan of this kind of complication to be honest. If you think you need nested tests or groups, you're probably better off splitting it up into multiple files (and get the benefit of parallelism) or simplifying your tests.

I'll keep this open for discussion, but even if we are going to add this, it's not going to happen right now. I want to focus our efforts on making a really good minimal core first.

sindresorhus avatar Nov 16 '15 16:11 sindresorhus

From a quick skim this looks like the same as #62 just with a different name.

Ah yes, apologies for not finding that myself.

even if we are going to add this, it's not going to happen right now. I want to focus our efforts on making a really good minimal core first.

Fair enough.


If you think you need nested tests or groups, you're probably better off splitting it up into multiple files (and get the benefit of parallelism) or simplifying your tests.

I'm writing some tests for functions that have reasonably complex behavior, based on stub objects that were set up in a beforeEach. Unfortunately these tests aren't easy to write or maintain. The best approach I've found is to define the various code paths in a single file with nested groups (using Mocha's describe/context helpers), each group inheriting the parent context and then refining it for the particular tests within it. Somehow breaking this up into many different files would just lead to a lot of repetition.

I'll see if I can think of a sane way to set up these tests with multiple files though.

novemberborn avatar Nov 16 '15 17:11 novemberborn

@novemberborn I'm not saying splitting up would work for every use-case and it does sound like yours is a difficult one. I just try very hard not to introduce new features until I'm absolutely certain it cannot be solved in a less complicated way. We'll still consider this, but we have a lot on our plates, and maintaining something like this with the level of code churn we currently have is not something I want to take on right now. Happy to continue discussing how it would look like, though.

sindresorhus avatar Nov 16 '15 17:11 sindresorhus

It's a pretty common need to set up a context, use it for a few tests, then have a nested describe to tune a few more things in the context and run a few more tests with the modified context.

For example, if one has a Project entity that needs to be created, then configured, then started, etc.:

describe('Project', () => {
  beforeEach(() => { this.project = new Project() });
  // a few tests verifying stuff about created project
  it('...', () => ...);

  describe('when configured', () => {
    beforeEach(() => this.project.configure());
      // a few tests verifying stuff about created and configured project
      it('...', () => ...);

      describe('when started', () => {
        beforeEach(() => this.project.start());
        // a few tests verifying stuff about created, configured, and started project
      });
  });
});

In a real code each step may contain much more to set up the context. While it's possible to express the same in a flat structure, it may require more code to do so and/or to introduce creating functions that otherwise could be avoided.

Anyway, whatever you end up doing or not doing, please consider not doing it the way it's done in tape.

The way it's done there (nested tests) makes it impossible to run a nested test without running an outer one. This is a limitation that may affect lots of future design decisions and integration options.

For example, if you ever decide to introduce a filter/grep for tests within a single file, you'd better have a conceptual entity for a group/suite of tests that is different from a test, otherwise it's not going to be possible.

ArtemGovorov avatar Nov 17 '15 07:11 ArtemGovorov

We'll still consider this, but we have a lot on our plates, and maintaining something like this with the level of code churn we currently have is not something I want to take on right now.

I appreciate that. I may be able to help once the code churn settles down.

novemberborn avatar Nov 17 '15 11:11 novemberborn

Thanks for all the use-cases. The more practical use-cases we get, the easier it is to evaluate a possible feature and how to implement it.

sindresorhus avatar Nov 17 '15 11:11 sindresorhus

I want to add one more little spin on groups.

Sometimes I have a group of identical tests that I want to run across a number of inputs. Examples include:

  1. My transform works with esprima / acorn / babel generated tree AST's.
  2. My promise based utility works with bluebird, pinkie, etc. (Maybe if offers some added bonus features if the more powerful bluebird API is available, but still works with smaller implementations).
  3. I offer different optimization options for some algorithm, but I want to verify they all produce identical output.

Using mocha, I usually accomplish this as follows:

function testWith(opts) {
  describe('with opts:  ' + JSON.stringify(opts), function () {
    it(....);
  });
}

testWith(optsA);
testWith(optsB);

If you want good examples of this, check out the power-assert test suite. This construct is all over the place. (cc: @twada).


I think it would be awesome if our groups API allowed something like this.

var bluebird = test.group('bluebird');
var pinkie = test.group('pinkie');

bluebird.beforeEach(t => t.context.Promise = require('bluebird'));
pinkie.beforeEach(t => t.context.Promise = require('pinkie'));

var both = test.group(bluebird, pinkie);

both.beforeEach(...);

both.test(...); // common test

bluebird.test(...); // bluebird specific test

This has a huge advantage over the method I described for mocha, in that I can easily make some tests specific to bluebird, while reusing the beforeEach setup I already have in place. In my mocha example above, any bluebird specific test would need to be declared outside the testWith function, and the beforeEach setup would need to be duplicated.

I still think we should provide a nesting API as well. With this in place, it would be pretty trivial:

var bluebird = test.group('bluebird', group => {
  // group === bluebird
});

jamestalmage avatar Nov 30 '15 03:11 jamestalmage

@jamestalmage To me the approach you've mentioned in mocha context looks cleaner.

In my mocha example above, any bluebird specific test would need to be declared outside the testWith function, and the beforeEach setup would need to be duplicated.

Doesn't have to be outside if your opts contain something to distinguish contexts:

function testWith(opts) {
  describe('with opts:  ' + JSON.stringify(opts), function () {
    it(....);

    // bluebird specific test
    opts.bluebird && it(....);
  });
}

testWith(optsA);
testWith(optsB);

ArtemGovorov avatar Nov 30 '15 03:11 ArtemGovorov

Well, let's flesh out a more complete, functionally identical example for both and see:

const bluebird = test.group('bluebird');
const pinkie = test.group('pinkie');
const q = test.group('q.js'); 
const all = test.group(bluebird, pinkie, q);

bluebird.beforeEach(t => t.context.Promise = require('bluebird'));
pinkie.beforeEach(t => t.context.Promise = require('pinkie'));
q.beforeEach(t => t.context.Promise = require('q'));

all.beforeEach('common setup', t => {
    // common setup
});

all('foo', async t => {
    t.ok(await itSupportsFoo(t.context.Promise));
});

bluebird('blue foo', t => {
    t.ok(blueFoo(t.context.Promise));
}); 

test(bluebird, q, 'no pinkie', t => t.ok(notPinkie(t.context.Promise)));
function testWith(opts) {
    test.group(opts.name, test => {
        opts.bluebird && test.beforeEach(t => t.context.Promise = require('bluebird'));
        opts.pinkie && test.beforeEach(t=> t.context.Promise = require('pinkie'));
        opts.q && test.beforeEach(t=> t.context.Promise = require('q'));

        test.beforeEach('common setup', t => {
            // common setup
        });

        test('foo', async t => {
            t.ok(await itSupportsFoo(t.context.Promise));
        });

        opts.bluebird && test('blue foo', t => {
            t.ok(blueFoo(t.context.Promise));
        });

        (opts.bluebird || opts.q) && test('no pinkie', t => {
            t.ok(notPinkie(t.context.Promise)); 
        });
    }); 
});

testWith({
    name: 'bluebird',
    bluebird: true
});

testWith({
    name: 'pinkie',
    pinkie: true
});

testWith({
    name: 'q.js',
    q: true
});

jamestalmage avatar Nov 30 '15 04:11 jamestalmage

Now imagine you have a larger test suite where you want to use this setup across multiple test files:

// _groups.js helper file

export const bluebird = test.group('bluebird');
export const pinkie = test.group('pinkie');
export const q = test.group('q');
export const all = test.group(bluebird, pinkie, q);

bluebird.beforeEach(t => t.context.Promise = require('bluebird'));
pinkie.beforeEach(t => t.context.Promise = require('pinkie'));
q.beforeEach(t => t.context.Promise = require('q'));
// test.js
import {bluebird, pinkie, q, all} from './_groups';

bluebird('blueFoo', ...);

I don't know what a convenient way to do this with wrapper functions would be.

jamestalmage avatar Nov 30 '15 04:11 jamestalmage

The second one can be expressed a bit shorter:

[
  { name: 'bluebird', bluebird: true }, 
  { name: 'pinkie', pinkie: true }, 
  { name: 'q', q: true }
].forEach(opts => {
    test.group(opts.name, test => {
        test.beforeEach(t => { 
            t.context.Promise = require(opts.name);
            // common setup
        });

        test('foo', async t => {
            t.ok(await itSupportsFoo(t.context.Promise));
        });

        opts.bluebird && test('blue foo', t => {
            t.ok(blueFoo(t.context.Promise));
        });

        (opts.bluebird || opts.q) && test('no pinkie', t => {
            t.ok(notPinkie(t.context.Promise)); 
        });
    }); 
});

And still looks simpler to me because:

  • group doesn't expose any API, but just serves as a simple container. So less things to learn.
  • context (set up in beforeEach) is easier discoverable, I just have to walk up the hierarchy. I'm worried that for a test like bluebird('blue foo', t => t.ok(...)); it may be harder to trace where the context is set. mocha-like context hierarchy may not look as sexy and dynamic, but it imposes the easily discoverable and traceable structure.

larger test suite where you want to use this setup across multiple test files

This doesn't sounds like a scenario I would often encounter. I'd prefer to have some smaller and simpler test suites with self-contained beforeEach hooks rather than spreading the setup logic over multiple files.

ArtemGovorov avatar Nov 30 '15 04:11 ArtemGovorov

        test.beforeEach(t => { 
            t.context.Promise = require(opts.name);
            // common setup
        });

While that optimization works here, there are plenty of scenarios where it won't. You may have very complex setups that differ widely (different database backends, parser API's that differ, etc).

I completely disagree on one being "easier to discover". I have seen plenty mocha test suites where it can be really confusing which nested group you are in:

describe('foo', () => {
  describe('bar', () => {
    beforeEach(...);

    // pages later

    it(...); // Wait, am I still inside 'bar' or not?

I'm not saying you couldn't find ways to abuse my proposal. Just that there is plenty of rope to hang yourself with either.

jamestalmage avatar Nov 30 '15 05:11 jamestalmage

I have seen plenty mocha test suites where it can be really confusing which nested group you are in

Discoverability is not just readability. With the example you've provided, tools can easily help. Your editor code folding, or jumping to/previewing the parent function would help. Even a simple static analysis tool (like a simple babel plugin), without executing your code, can build you a nice tree showing how things are nested. Combining things dynamically on the other hand makes it from hard to impossible to view the structure before executing the code.

I think your idea is certainly more flexible in terms of allowing to dynamically combine context than any imposed structure. It's not that it's easier to abuse, because anything can be abused.

It's just that when I open a test suite written in mocha/jasmine, I know what to expect there and where because of the static structure that everyone follows. With the dynamic context setup, I can easily see many different people doing it very differently creating a learning barrier for new contributors.

ArtemGovorov avatar Nov 30 '15 05:11 ArtemGovorov

To be clear, nesting would still be supported in my proposal. You could certainly elect to stick with it. I just strongly dislike look of the extra nesting required with your approach. I just feel my is cleaner and more readable.

If we were to implement my idea. I think a "best practice" would be to name your groups at the top of a file. I can't see how it would be much harder to write a Babel plugin to analyze my proposed structure (though I grant you it would be a little harder). That said, it also wouldn't be hard to expose the structure by emitting an event prior to firing off tests (if some of the concerns you are raising are wallabyjs related).

jamestalmage avatar Nov 30 '15 06:11 jamestalmage

how it would be much harder to write a Babel plugin to analyze my proposed structure

The issue is that you can't enforce the "best practice" structure because you're suggesting to expose an API, potential usages of which are limiting static analysis options. For example, please correct me if I'm wrong, but with your suggested API one can do:

export const bluebird = test.group('bluebird');
export const pinkie = test.group('pinkie');
export const q = test.group('q');

let someGroups = [];
someLogic && someGroups.push(q);

export const some = test.group(...someGroups);

// Try creating a babel plugin that would tell groups of the test:
some.test(...);

it also wouldn't be hard to expose the structure by emitting an event prior to firing off tests (if some of the concerns you are raising are wallabyjs related).

My concerns are not wallaby related (it's actually structure agnostic and doesn't rely on static analysis of test structure for its essential functionality). Just thoughts from a user prospective rather that from an extender point of view.

ArtemGovorov avatar Nov 30 '15 07:11 ArtemGovorov

I could perform the same hacks to the array in your mocha example, creating the same difficulties.

My concerns are not wallaby related

Then why are you bringing up the Babel plugin? Is that something someone would use IRL?

jamestalmage avatar Nov 30 '15 07:11 jamestalmage

@jamestalmage Mocha doesn't expose a way to combine groups via API, but you are suggesting to. You can perform "hacks" to abuse mocha structure, but in the case of your suggested API it is not in hack - test.group(...someGroups);, it's a normal use. And the normal use makes it impossible to run static analysis and determine your tests structure.

Then why are you bringing up the Babel plugin? Is that something someone would use IRL?

I have used Babel as an example of a tool that does static analysis. But generally yes, there are tools that use static analysis to highlight mocha/jasmine tests, run them by full name (that they can determine via static analysis in most of cases). Real life example is WebStorm IDE.

ArtemGovorov avatar Nov 30 '15 08:11 ArtemGovorov

@jamestalmage here is the real life example: w I think I have seen similar plugins in other editors as well.

ArtemGovorov avatar Nov 30 '15 08:11 ArtemGovorov

@sindresorhus: For some context, the way I usually use describes in mocha is (for example), if I'm testing an object, I'll have one describe per method, and various tests within that. That's nice because you can easily see the hierarchy, you get code folding, and the output is formatted nicely.

ariporad avatar Dec 23 '15 05:12 ariporad

I'm thinking of an implementation like this:

test.group(function(test) {
  test("test a", function() {
  });
  test.beforeEach("before test a but not test b", function() {
  });
});
test("test b", function() {});

Basically test.group just generates another runner that is added into the Runner.prototype.tests array. All of the hooks work as expected, just with the new test in context.

matthewbauer avatar Jan 25 '16 21:01 matthewbauer

Runner.prototype.tests

Well... not on the prototype hopefully. :smile:

But yes, that is basically what is being discussed here. You should definitely hold off on creating a PR until:

  1. #466 lands
  2. We get consensus here on the API (it's still not settled that we actually want to add it).

jamestalmage avatar Jan 25 '16 21:01 jamestalmage

Well... not on the prototype hopefully. :smile:

Woops I was thinking of that backwards. I meant "runner.tests" where runner is an instance of Runner.

But yes, that is basically what is being discussed here. You should definitely hold off on creating a PR >until:

#466 lands We get consensus here on the API (it's still not settled that we actually want to add it).

Definitely.

The big thing with this is that without some way to group tests there are a whole class of tests that you can't really work with. For my use case:

I have an emulator that needs to be tested. I have written common tests that I want to run for each ROM in my fixtures. I don't want to have to write a test file for each ROM because they should all have the same tests (render 50 frames, check video size, audio checks...) and I'd just be doing the same thing over and over. But I also need for a fresh ROM to be loaded for each individual test. So I need some way to make the "beforeEach" run differently depending on which ROM I'm testing.

matthewbauer avatar Jan 25 '16 22:01 matthewbauer

You might have good reason to want individual files. Each separate file gets it's own process, and runs concurrently, so there might be some speed benefits.

jamestalmage avatar Jan 25 '16 22:01 jamestalmage

My current day job is working with one team on a larger angular app written by multiple teams. We test with karma and jasmine. Our component classes (controllers and services) have a history of being larger than necessary. Our team is trying to reduce the size and complexity of our new components. It's also our first attempt at any kind of TDD/BDD, so we are learning a lot.

Our application structure boils down to (with 30+ components):

app/
- componentA/
 - componentA.js
 - componentA.html
 - componentA.Test.js
 - componentA.Ctrl.js
 - componentA.Ctrl.Test.js
 - componentA.Svc.js
 - componentA.Svc.Test.js
- componentB/...
- ...
- componentZ/...

So that's currently 3 test files already per component.

We structuring tests is to describe what is being tested, then describe each instance method with both a pass and fail test plus any other extra cases that need to be covered.

Test structure follows something similar to:

describe('Component A Controller', function() {
  beforeEach(...)

  describe('instanceMethod1', function() {
    it('should have a happy path', function() {
    })
    it('should have a sad path', function() {
    })
    it('should meet a range of user story requirements', function() {
    })
  })

  describe('instanceMethod2', ...)

  describe('instanceMethodN', ...)
})

One benefit of this method is we get nice mapping of tests to user stories with a clear report for the broader team to view (BA's, managers, test team etc). The output of the report reads like:

Component A Controller instanceMethod1 should reach some desired outcome

There's probably a lot of thing here that could be improved/structured differently. That's my experiance

BarryThePenguin avatar Jan 25 '16 22:01 BarryThePenguin

+1

I am glad we all agree on the benefit of nested tests. Hope to see this implemented soon!

dbkaplun avatar Feb 14 '16 14:02 dbkaplun

Probably nested tests may be useful in some situations, however I fear they may be a problem for parallelism and in general they introduce dependencies and context that start to get quite hidden and cryptical as the test suite grows.

Before implementing test groups I think it would be really helpful to collect some opinionated alternatives to them that may encourage context-free and parallel-friendly tests.

e.g. if you have this entity Project and need to test different things when it is initialized, configured or started... you may have a factory function like this:

function getProjectMock(opts) {
  const project = new Project();
  opts.configure && project.configure();
  opts.start && project.start();

  return project;
}

This way the only repetition you have is calling the factory function that in my opinion is a "good repetition" as it somehow document the actual mocked object you are using in each test.

Beside that you always have a freshly created Project instance without the risk of sharing unwanted internal states.

Said that I strongly believe that enforcing pure functions and avoid context sharing on Object methods is usually a good thing, but of course it is not always the case and you may have existing codebase that use a lot of shared globals or being in need of some complex transactions that may be easier to express using a shared state... so groups may still be a useful feature, but to be somehow "discouraged" showing possible more idiomatic alternatives for various common cases.

axyz avatar Feb 25 '16 09:02 axyz

fear they may be a problem for parallelism context sharing

If test groups define tests and hooks (and not execute them immediately), then it should not affect parallelism at all and there's no context sharing.

Let's take my example:

describe('Project', () => {
  beforeEach(() => { fixture.project = new Project() });
  // a few tests verifying stuff about created project
  it('1', () => ...);

  describe('when configured', () => {
      beforeEach(() => fixture.project.configure());
      // a few tests verifying stuff about created and configured project
      it('2', () => ...);

      describe('when started', () => {
        beforeEach(() => fixture.project.start());
        // a few tests verifying stuff about created, configured, and started project
        it('3', () => ...);
      });
  });
});

All of these tests can be executed in parallel. It's just before the test 1 (and each tests for this level) only

fixture.project = new Project()

will be executed. For the test 2 (and each test for this level):

fixture.project = new Project()
fixture.project.configure()

will be executed, etc.

Put simple, test groups is just a convenient way to organise tests and context creation. And I don't think it really matters what kind of code you are testing: functional, procedural or object oriented. When testing a function, no matter pure or not, you will need to create and pass some state. Sometimes you may want to re-use the state creation code and test groups allow you to do it in a less verbose manner.

ArtemGovorov avatar Feb 25 '16 10:02 ArtemGovorov

Just wanted to leave a random thought here for future API implementation: In Mocha, it's nice that you can opt not to define an its testing implementation and it will show up as a placeholder:

it('should do something i have not implemented yet')

But it's frustrating that you can't do the same thing for a describe call. If the groups implementation ends up looking like some of these examples, it would be nice is groups too can have their implementation functions omitted and show up as placeholders.

ianstormtaylor avatar Mar 06 '16 22:03 ianstormtaylor

What problem is solved by adding nested groups? For me, the problem is isolating setup/teardown hooks to specific tests. I don't want to create a separate test file just to have separate lifecycle hooks. For example, I may have one test that stubs one utility function and another tests that stubs something else. These test need different setup/teardown logic, which is where nested groups come in handy to manage it. It's a constant annoyance to me that I have to ensure that my one 'afterEach' hook provides logic that supports the one test that needs it without breaking all the other tests.

I think there's a better (simpler) way than adding nesting. How about we just register the teardown hook as part of our test? All we would need to do is add a single method to the api: t.after(func).

Consider the following example:

test('getCwd() returns the current working directory', t => {
  sinon.stub(utils, 'doSomething').returns('/fake/path'); // setup
  t.after(() => utils.doSomething.restore()); // teardown

  t.is(getCwd(), '/fake/path');
});

With more tests, this might be refactored into functions so the setup/teardown logic can be reused:

const setup = () => sinon.stub(process, 'cwd').returns('/fake/path');
const teardown = () => process.cwd.restore());

test('getCwd() returns the current working directory', t => {
  setup();
  t.after(teardown);

  t.is(getCwd(), '/fake/path');
});

// ... lots more tests

If you don't like that, perhaps another simple solution is just to return a promise from test(). The promise would resolve whether it failed or not, so the code would look like this:

test('getCwd() returns the current working directory', t => {
  setup();
  t.is(getCwd(), '/fake/path');
}).then(teardown);

spudly avatar Mar 15 '16 13:03 spudly

@spudly I like that! Another option would be to use .after to register an after handler without having to make test return a promise:

test('getCwd() returns the current working directory', t => {
  setup();
  t.is(getCwd(), '/fake/path');
}).after(teardown);

One nicety of groups though would be to use a beforeEach that modifies t.context. You could chain those and not have to worry about calling setup() in each test.

novemberborn avatar Mar 15 '16 14:03 novemberborn

I've never really understood the use case for t.context (or using this inside mocha tests)... what does t.context give me that I can't accomplish with a simple variable?

spudly avatar Mar 15 '16 16:03 spudly

Another option would be to use .after to register an after handler without having to make test return a promise

After giving this some thought, I think this is the best approach. Perhaps you could also have a .before method as well and provide context to those two callbacks. Here's another contrived example, this time with context.

const setup = t => {
  t.context.realCwd = process.cwd;
  process.cwd = () => '/fake/path';
};

const teardown = t => {
  process.cwd = t.context.realCwd;
};

test(t => {
  t.is(getCwd(), '/fake/path');
}).before(setup).after(teardown);

The obvious drawback here is that you have to include .before(setup).after(teardown) on each test instead of for the entire group, but I personally prefer that because it's more explicit.

Based on the way test is currently implemented, it looks like the implementation would be chainable, so you could probably do something like the following:

test
  .before(setup)
  .test(t => t.ok(t.context.foo))
  .after(teardown);

We may have to create new functions instead of reusing the existing .before() and .after() chainable methods...

spudly avatar Mar 15 '16 23:03 spudly

@spudly

What problem is solved by adding nested groups?

For example this one https://github.com/sindresorhus/ava/issues/222#issuecomment-157299689.

For me, the problem is isolating setup/teardown hooks to specific tests.

If that's the only problem that you'd like to address, then why bothering adding any API at all? How what you are suggesting:

const setup = t => {
  ...
};

const teardown = t => {
  ...
};

test(t => {
  t.is(getCwd(), '/fake/path');
})
.before(setup)
.after(teardown);

is better than simply doing this?:

const setup = t => {
  ...
};

const teardown = t => {
  ...
};

test(t => {
  setup(t);
  t.is(getCwd(), '/fake/path');
  teardown(t);
});

ArtemGovorov avatar Mar 16 '16 01:03 ArtemGovorov

If I do that, the teardown function won't get called when the test fails. That would likely cause lots of other tests to fail and make it difficult to debug.

On Tue, Mar 15, 2016, 9:04 PM Artem Govorov [email protected] wrote:

@spudly https://github.com/spudly

What problem is solved by adding nested groups?

For example this one #222 (comment) https://github.com/sindresorhus/ava/issues/222#issuecomment-157299689.

For me, the problem is isolating setup/teardown hooks to specific tests.

If that's the only problem that you'd like to address, then why bothering adding any API at all? How what you are suggesting:

const setup = t => { ... }; const teardown = t => { ... }; test(t => { t.is(getCwd(), '/fake/path'); }) .before(setup) .after(teardown);

is better than simply doing this?:

const setup = t => { ... }; const teardown = t => { ... }; test(t => { setup(t); t.is(getCwd(), '/fake/path'); teardown(t); });

— You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub https://github.com/sindresorhus/ava/issues/222#issuecomment-197092754

spudly avatar Mar 16 '16 01:03 spudly

@spudly Fair enough, missed the case.

ArtemGovorov avatar Mar 16 '16 01:03 ArtemGovorov

I've never really understood the use case for t.context (or using this inside mocha tests)... what does t.context give me that I can't accomplish with a simple variable?

A simple variable doesn't work with AVA if you have async tests, consider this example:

import test from 'test';
import delay from 'delay';

var a;

test.beforeEach(() => a = 'foo');

test(async t => {
  await delay(20);
  t.is(a, 'foo');
});

test(async t => {
  await delay(10);
  t.is(a, 'foo');
  a = 'bar';
});

The first test is going to fail, because a will be set to "bar" while it awaits the completion of delay(20).

jamestalmage avatar Mar 17 '16 02:03 jamestalmage

For anyone interested in groups for ava, I've created ava-spec: https://github.com/sheerun/ava-spec

sheerun avatar Apr 18 '16 13:04 sheerun

FWIW, using files as groups works wonderful for me (I didn't know AVA uses file name when reporting errors 🎉). I don't think this feature is necessary.

silvenon avatar May 28 '16 13:05 silvenon

I've always liked the nested describe/it style as much for the reporting style it enables as much as anything else i.e.

ComponentA:
  does this
  does that
  when its raining:
    does something else

cant imagine this is considered valuable by many but it just seemed to make testing more meaningful to me.

halhenke avatar Jun 22 '16 14:06 halhenke

I've gotten considerably more experience writing tests in AVA than I had when I raised this issue back in November. I've now come round to the idea that nesting tests is an anti-pattern. Keeping the test hierarchy in your head takes considerable effort, especially in larger test files. Breaking up tests across multiple files is less obvious because that breaks the hierarchy. It seems easier to write tests for tests sake.

If this reeks too much of personal feelings that can't be backed with hard evidence then rest assured there's also a more technical reason it's difficult for AVA to support nested tests: it starts executing all your tests at once. Consequently it can't output a test hierarchy in real time. Taking @halhenke's example, AVA might give you the following output:

ComponentA:
    does something else
  does that
  when its raining:
  does this

ava-spec works by repeating the hierarchy on each line:

ComponentA:  does this
ComponentA:  does that
ComponentA:  when its raining: does something else

So if you prefer the test syntax ava-spec is the best solution for you.

(Also, AVA already prefaces the test output with the filename, so you don't even need to mention it in your test titles.)


The above doesn't help if you're nesting your tests in order to for those tests to have ever more refined context.

Again, nesting can lead you down an unnecessarily complicated path. In many cases simple helper methods are sufficient. Test macros are also very useful.

But, rightfully complex test suites do exist. Composing helper functions is possible but problematic, especially if you need to clean up part of the context after tests run.

I'm currently experimenting with ways to set up these context hierarchies separately from the test hierarchy. (Incidentally for the project that caused me to open this issue in the first place.) I'll check back in here once that's further along.

novemberborn avatar Jun 25 '16 16:06 novemberborn

@novemberborn Hey. I have to say i didn't find the examples from the provided link to be very convincing in terms of nested (I'd probably prefer the term "grouped") tests as an anti-pattern. For whatever reason the Mocha style code had big messy comments that line-wrap & generally help to try and make it look more confusing than the other example. If anything it seemed to be an argument against the dangers of beforeEach/afterEach use/abuse.

Can't really comment on the test execution thing. I guess my comment mostly stemmed from the fact that the Mocha/RSpec file seems to provide a nice way to organise/describe what you are testing & from what I've seen of Tape/AVA the alternative is to shove more info into the title string. It may be a very superficial thing and/or maybe just poor usage on my/others part but it is something that i miss.

halhenke avatar Jun 25 '16 17:06 halhenke

I guess my comment mostly stemmed from the fact that the Mocha/RSpec file seems to provide a nice way to organise/describe what you are testing & from what I've seen of Tape/AVA the alternative is to shove more info into the title string. It may be a very superficial thing and/or maybe just poor usage on my/others part but it is something that i miss.

@halhenke fair enough. Hopefully ava-spec will work for you. Once we figure out a good technique for creating context hierarchies tools like ava-spec can help with nesting beforeEach calls etc.

novemberborn avatar Jun 25 '16 17:06 novemberborn

nested tests are a good thing - if you take what would be nested and put it into separate test files, you will definitely see backwards progress in terms of overall speed. Allowing for nested stuff gives the developer choices - and the ability to put stuff that should be in the same file, into the same file. There are good patterns that you can use with nested describe blocks, that you can't without them. Here is a perfect example:

https://gist.github.com/ORESoftware/e5ed7161d443c69730ae1dd1a9997a6a?ts=4

const suman = require('suman');
const Test = suman.init(module);

Test.describe('Test inits', {parallel: false}, function (Pool, assert, path) {

    const data = {
        size: 5,
        filePath: path.resolve(__dirname + '/fixtures/sample-file.js')
    };

    const pool = new Pool(data);
    var size = pool.getCurrentStats().all;

    this.describe('#remove workers', function () {

        this.beforeEach(function () {
            pool.removeWorker();
        });

        for (var i = 0; i < 5; i++) {
            this.it(function () {
                assert.equal(pool.getCurrentSize().all, --size);
            });
        }

    });

    this.describe('#add workers', function () {

        this.beforeEach(function () {
            pool.addWorker();
        });

        for (var i = 0; i < 5; i++) {
            this.it(function () {
                assert.equal(pool.getCurrentSize().all, ++size);
            });
        }

    });

    this.after(function(){
        process.nextTick(function(){
            pool.removeAllListeners();
        });
    });

});

as you can see if the beforeEach hooks applied to all test cases in the test file, then we couldn't do something even as simple/intuitive as the above. Pretty lame if you don't have nested blocks, and boring.

ORESoftware avatar Jun 26 '16 05:06 ORESoftware

@novemberborn Yeah to be honest I'm a little ashamed to say that I only glanced at ava-spec before but it looks like it addresses what I'm talking about really well. With a reporter that generates similar output to the documentation format flag in RSpec i'd be very happy 😃

halhenke avatar Jun 26 '16 10:06 halhenke

hi, since this issue created last year, is there any implementation ready for preview?

I'm in a project want to switch testcases to ava (from mocha) to write new cases, but all existing test files are written in mocha describe it style well organized in a hierarchy, looks like will be blocked by this #222; wonder is there a simple drop in replacement that doesn't require me to rewrite all describe it ?

c0b avatar Aug 01 '16 16:08 c0b

@c0b have a look at https://www.npmjs.com/package/ava-spec. It doesn't support everything but maybe it's sufficient for your use case.

novemberborn avatar Aug 01 '16 17:08 novemberborn

I pretty much agree with @ORESoftware

I used mocha in a previous to test a loopback server application, and I looked at AVA mainly for the "each test (suite) runs in its own process", because properly clearing up the server/npm cache between test suites was starting to be a nightmare.

So for this AVA is a big winner, however, the lack of nested tests or at least hierarchical ordering of information inside the test report is a bigdownside.

I find this

Some API model
   Doing POST /foo should do this
   Doing DELETE /foo should do that
   etc

much clearer than

Some API model Doing POST /foo should do this
Some API model Doing DELETE /foo should do that
etc

I don't think it's possible to get the first output with ava-spec, can you confirm @sheerun ?

Other than that, AVA looks close to perfect so I do hope some solution can be found

Overdrivr avatar Aug 30 '16 13:08 Overdrivr

Not possible unless someone figures out interactive output formatter a'la less. Tests are run asynchronously, so it's impossible to group them and at the same time output logs line-by-line.

sheerun avatar Aug 30 '16 15:08 sheerun

  • That doesn't seem to be problem, you could just output ${ testGroup } - ${ testName }... for these line-by-line test results
  • For CI/whatever you could have different reporting which would print the results after all tests are done, so the output can be nicely formatted and nested like in mocha

Hurtak avatar Aug 30 '16 15:08 Hurtak

For me, I really like having tests aligned with one assertion. So something like:

test.group('When modifying something in particular', t => {
  t.context.store.dispatch(someMutatingAction())
  test('x becomes true', t => t.true(t.context.store.getState().x))
  test('y becomes 1', t => t.is(t.context.store.getState().y, 1))
}

is far less verbose than something like:

const doMutatingAction = t => t.context.store.dispatch(someMutatingAction())

test('When modifying something in particular x becomes true', t => {
  doMutatingAction(t)
  t.true(t.context.store.getState().x))
})

test('When modifying something in particular y becomes 1', t => {
  doMutatingAction(t)
  t.is(t.context.store.getState().y, 1)
})

And I don't want to use .before here of course because I don't want this mutation applied to all my tests in the file, and I'd also rather not create many different files for something that is pretty well contained (such as testing a single action or reducer, as in that case).

And as others have mentioned, I also enjoy the ability to setup a context for a subset of tests without resorting to different files, as there's definitely cognitive overhead involved in file switching.

I think it all ultimately comes down to the need to pass along a context, which can only be adjusted with before hooks (or by using globals for static things), which can only be declared once per module, a seemingly arbitrary constraint. Regardless of whether the nesting was used as an "antipattern" or not, grouping will add needed expressivity to this library.

gravitypersists avatar Sep 01 '16 00:09 gravitypersists

I ported my Buoyant project from Mocha to AVA. It's this project that made me open this issue in the first place.

I found that explicitly calling helper functions in my tests, as well as using test macros, removed most of my need for nesting. The flat structure actually helped reduce repetition and made it easier to grok what was being tested. That said the tests are still really complex, but IMO that's due to what I'm trying to test. (For those looking at the test code, I did overlook that test() can take an array of macros…)

I did have some use cases where I wanted to refine test context and run tests within that refined context, see example. I wrote some helper code to allow forking the context.

If we implement this in AVA the interface would be something like:

import test from 'ava'

test.beforeEach(t => {
  t.context.forAllTests = true
})

const refined = test.fork().beforeEach(t => {
  t.context.refinedTestsOnly = true
})

test(t => {
  t.true(t.context.forAllTests)
  t.false(t.context.refinedTestsOnly)
})

refined.test(t => {
  t.true(t.context.forAllTests)
  t.true(t.context.refinedTestsOnly)
})

ava-spec would also be able to use this infrastructure to implement nested beforeEach() calls etc.

@avajs/core thoughts?

novemberborn avatar Sep 21 '16 14:09 novemberborn

That reminds of Elixir test tags so I'm all for it. Looks clean.

sotojuan avatar Sep 21 '16 14:09 sotojuan

Here's a recap of all proposed APIs, I think it'll make it easier to pick a winner and continue the discussion.

1. test.group() with nesting by @novemberborn

test.beforeEach(t => {
  t.context.forAllTests = true;
});

test.group('some nested tests', () => {
  test.beforeEach(t => {
    t.context.forGroupTests = true;
  });

  test(t => {
    t.true(t.context.forAllTests);
    t.true(t.context.forGroupTests);
  });
});

test(t => {
  t.true(t.context.forAllTests);
  t.falsy(t.context.forGroupTests);
});

Pros:

  • API is easy to learn/use and is already familiar to most people

Cons:

  • Nesting - tests are harder to find (see https://github.com/avajs/ava/issues/222#issuecomment-160516588)

2. Groups with no nesting and ability to mix together by @jamestalmage

var bluebird = test.group('bluebird');
var pinkie = test.group('pinkie');

bluebird.beforeEach(t => t.context.Promise = require('bluebird'));
pinkie.beforeEach(t => t.context.Promise = require('pinkie'));

var both = test.group(bluebird, pinkie);
both.beforeEach(...);
both.test(...); // common test

bluebird.test(...); // bluebird specific test

Pros:

  • No nesting, continues to maintain "AVA style"
  • Flexibility and hooks reuse

3. Groups with nesting and overriden test function by @matthewbauer

test.group(test => {
  test.beforeEach('before test a but not test b', () => {});

  test('test a', () => {});
});

test('test b', () => {});

Similar to nr. 1, except test is overriden inside each group (which may be a good thing actually, for an easier implementation).

4. .before() and .after() test modifiers by @spudly

const setup = t => {
  t.context.realCwd = process.cwd;
  process.cwd = () => '/fake/path';
};

const teardown = t => {
  process.cwd = t.context.realCwd;
};

test(t => {
  t.is(getCwd(), '/fake/path');
}).before(setup).after(teardown);

Pros:

  • Explicitness
  • Probably least complex implementation
  • Minimum of the new API introduced

Cons

  • Need to repeat .before() and .after() for each test. But, with those modifiers implemented, it may be very easy to implement something like t.group(), which applies these hooks to all nested tests. It would even make it possible to implement as a separate module, outside of AVA core.

5. test.fork() without nesting by @novemberborn

test.beforeEach(t => {
  t.context.forAllTests = true
});

const refined = test.fork().beforeEach(t => {
  t.context.refinedTestsOnly = true;
});

test(t => {
  t.true(t.context.forAllTests);
  t.false(t.context.refinedTestsOnly);
});

refined.test(t => {
  t.true(t.context.forAllTests);
  t.true(t.context.refinedTestsOnly);
});

Pretty much similar to nr. 2, but with .fork() instead of .group() (I find latter more understandable) and without mixing of groups (less complexity -> less bugs and maintenance - good thing).


Now that we have proposals side by side, hope it makes it easier for everyone to choose what they prefer.

vadimdemedes avatar Sep 29 '16 08:09 vadimdemedes

I think the best thing about the first solution is the nesting and it's familiarity. It's a visual cue that you're going deeper and adding complexity. I disagree with your con. I feel like that makes it more difficult to miss tests.

I like the flatness of .fork and the ability to compose tests with .group, but I feel these could be much harder to see visually and reason about in the future. I also think they make it easier to hide code but not in a desirable way.

We're writing code for developers to read. It's secondary that computers can interpret it.

If I were to open a mocha test file right now, I could find every context that's nested 3 levels deep in my editor /^\s{6}describe/ and visually. There's nothing distinct about .fork or .group to highlight that complexity. I could potentially have helpers that fork that I wouldn't even see in the same file. It feels like it allows things to get overly complex easily but not in an obvious way. When I'm 10 levels of nested describes deep, you know it. You feel it. (You're probably crying by then.) I could fork 10 times in a helper and then 5 more times in my test and it may not be obvious the person reading my code. I could see me having to open up several files just to figure out what the setup for one test looks like.

Nested tests feels a bit like safety rails/guards. Even if you don't describe your test setup well, I can still usually take it all in one file and reason about it. The other 2 flat solutions feel a bit footgun-y to me. They open up a lot of options, but with that comes a lot of responsibility. They place the onus on the developer to name things well and be very descriptive: updateFailedAndBeforeRetryRequest.test vs complexSetup.test(...). Even the "well named" solution in my example feels bad to me. That's just my 2 cents.

fearphage avatar Oct 02 '16 10:10 fearphage

My vote is for 2 or 5. Flatness wins to me, otherwise we'd never see async/await.

Akkuma avatar Oct 11 '16 02:10 Akkuma

I'd second @Akkuma on 2 or 5, leaning towards 5 😉

jrolfs avatar Oct 11 '16 13:10 jrolfs

I'm pro number 1 or 2 but would love to see some sort of test grouping ability introduced. I really miss that from mocha.

hasLandon avatar Oct 17 '16 22:10 hasLandon

I would like to point out that you can already get grouping with pretty low effort:

const groupTest = name => {
  let sharedPromise
  const withShared = fn => t => {
    if (!sharedPromise) {
      sharedPromise = getPromised()
    }
    return fn(sharedPromise, t)
  }

  test(`${name}: test1`, withShared(async (sharedP, t) => {
     t.is(await sharedP, 5)
  }))
  test(`${name}: test2`, withShared(async (sharedP, t) => {
     // ...
  }))
}
groupTest('group1')
groupTest('group2')

You can even make the withShared simpler with a helper:

export const sharedSetup = getPromise => fn => {
  let promise
  return await t => {
     if (!promise) {
       promise = getPromise()
     }
     return fn(await promise, t)
  }
}
const withShared = sharedSetup(() => openedDatabasePromise())
test('meep', withShared((db, t) => /*...*/))

so maybe any added API surface would be limited to helpers?

wmertens avatar Nov 13 '16 11:11 wmertens

Nested tests are necessary for testing the success of a function that wraps a test.
For instance, I have a wrapper that takes a function with metadata containing examples, and tests the examples:

import test from 'ava'

const META = Symbol('META')

function add(a, b){
  return a + b
}
add[META] = {
  description: 'adds two numbers',
  examples: [{ input: [ 1, 2 ], output: 3 }]
}

function testExamples(f){
  let { description, examples = [] } = f[META] || defaults(f)
  test(description, t => {
    examples.forEach(({input, output}) => {
      if(typeof output == 'object'){
        t.deepEqual(f(...input), output);
      } else {
        t.is(f(...input), output);
      }
    })
  })
}

testExamples(add)

I'd like to test this with ava, but can't without nested testing:

test('testing examples works', t => {
  let add = (a, b) => a + b
  add[META] = {
    description: 'adds two numbers',
    examples: [{ input: [ 1, 2 ], output: 3 }]
  }
  testExamples(add)
  add[META] = {
    description: 'adds two numbers',
    examples: [{ input: [ 1, 2, 3 ], output: 6 }]
  }
  t.throws('invalid example throws', _ => testExamples(add), AssertionError)
})

micimize avatar Jan 02 '17 21:01 micimize

@micimize perhaps you can test it by mocking ava?

mightyiam avatar Jan 03 '17 17:01 mightyiam

@micimize Your use case seems perfectly suited for AVA’s test macros.

sonicdoe avatar Jan 04 '17 22:01 sonicdoe

My vote is actually to have two of the suggested options rather than just one (both option 1 and option 2 from the options listed in https://github.com/avajs/ava/issues/222#issuecomment-250399133).

I think option 1 is almost necessary as many people really like using it and think of their tests most naturally in a nested way, and it will make it easier for people to migrate to ava. And option 2 is great for people who don't like nesting and want something different, modern, and 'cleaner' than what's already out there in various test frameworks.

In fact I can see most people starting off with option 1 when they migrate to ava, and eventually switching to option 2 as they adopt the new paradigm. In fact, this is also my own intention with our projects.

I think limiting the solution to just one of the choices would leave a large segment of users feeling like they're constrained by the test framework. Which I think is a big part of why so many of us immediately took a liking to ava, coming from other frameworks. But having both of these would be a great solution. I'm not sure how feasible it is from a coding perspective to make that happen, but in my opinion it would be the way to go.

rightaway avatar Jan 06 '17 13:01 rightaway

not sure what the status is on this one, but I am a fan of nested groups, I already added an example on this thread that demonstrates the usefulness of nested groups (it was awhile ago), but here is another example that just came up in my real life, where nested groups feel very natural:


const suman = require('suman');
const Test = suman.init(module, {
    pre: ['create-test-dir']
});


Test.create('backpressure', (Queue, Rx, path, fs, before, it, suite, util, userData, after, describe) => {

    const id = suite.uniqueId;
    const pre = userData['suman.once.pre.js'];
    const p = pre['create-test-dir'];

    const q = new Queue({
        port: 3500,
        fp: path.resolve(p + '/spaceships' + id + '.txt'),
    });

    const count = 9;  // expected count

    before('add items to queue', h => {                    // 1st before block
        return Rx.Observable.range(0, count)
            .map(function (i) {
                return q.enq('frog(' + i + ')');
            })
            .concatAll();
    });

    it('has correct count', t => {             //  1st test case

        return q.getSize()
            .do(function(data){
                if(data.count !== count){
                    throw(' => Count is incorrect.');
                }
            });

    });


    // here is our nested block/group

    describe('get count after draining queue', (before, it) => {

        before('drains queue', h => {              //  2nd before block

            return q.drain()
                .backpressure(function(data,cb){
                    setTimeout(cb,500);
                });
        });

        it('has correct count', t => {             // 2nd test case

            return q.getSize()
                .do(function(data){
                    if(data.count !== 0){
                        throw(' => Count is incorrect.');
                    }
                });

        });

    });


});

as you can see there are two test cases and two before blocks. The second before block only runs before the second test, it does not run before the 1st test, and that's the key idea. If there were no nesting, then the second before block would run before both tests, which is not what we want. The above gives the dev way more control and expressivityity.

The above is the natural way to create a test, any other organization probably is less natural, and therefore less intuitive. Just my 2 cents.

Note that this is the same exact need /use case as my prior example, where we only want before blocks to run before select tests cases, not all test cases.

ORESoftware avatar Jan 10 '17 01:01 ORESoftware

@mightyiam my current approach is just testing with node-tap, which I guess is similar @sonicdoe a custom macro would definitely make my code cleaner in a lot of respects, but doesn't solve the fundamental "test a test" problem.

micimize avatar Jan 11 '17 04:01 micimize

apologies if this is possible in a different way, but for me, this would be interesting for async creation of test cases:

import glob from 'glob-promise'
import fs from 'mz/fs'

test(fixturedir, async t => {
	const fixtures = await glob(`${fixturedir}/*`)
	// now i can actually create the tests since the glob has finished
	for (const fixture of fixtures) {
		t.test(fixture, async t => {
			const names = ['source', 'target'].map(f => path.join(fixture, f))
			const [source, target] = await Promise.all(names.map(fs.readFile))
			t.is(source, target)
		})
	}
})

flying-sheep avatar Jan 11 '17 10:01 flying-sheep

Just my two cents - I use "describe" functionality mostly to group my tests so that they appear grouped in my reporter. I generally will create a test file for a module, and I will use describe grouping for specific subsets of functionality. (Often, this is actually split by method, but it can be split in other ways as well.) The individual tests will test different conditions and inputs for a given output (general cases, edge cases, what happens if something is null, etc.)

I know this isn't the general consensus but I wouldn't mind a simple describe functionality that had no effect on the tests themselves - only the reporter. More functionality would be nice, but I could get away with this for my needs.

jwahyoung avatar Mar 09 '17 23:03 jwahyoung

While we're sharing usage, my describe usage is more about state management and steps in a process. This is the pseudo code version of many of our tests.

describe('upload view') {
  beforeEach('create view + fake server')
  it('should make a request on submit');

  describe('after submit') {
    beforeEach('submit upload form');

    it('should handle failure');
    it('should handle success');
    
    describe('when upload fails') {
      beforeEach('respond with failure from fake server')
 
      it('should retry');

    describe('when upload succeeds') {
      beforeEach('respond with 201 from fake server')

      it('should redirect to new view');

As tests become more deeply nested, they are adding new steps to the process building on the previous state. That's why the first suggestion resonates so much with me. It's very visual like mocha tests. It's not trying to hide/obscure the complexity.

fearphage avatar Mar 10 '17 14:03 fearphage

Here's another example that's kind of similar to what @fearphage mentioned above.

My group often uses gherkins to structure our tests. We have some pretty hairy business requirements, so our tests also serve as documentation of behaviour. We follow TDD and also do a bit of BDD, so we also try to use gherkins in our requirements to loosely capture the details of user stories. It sounds like a lot of overhead, but we've found it very useful guide our TDD process. This thought-experiment-turned-pseudo-practice may not be for everyone, but it's proven pretty useful for our group and wouldn't be possible without nested groups.

I'll use a non-async example, in this case, validation for some imaginary input field:

describe('Given input for my overly prescriptive input validator', () => {
    beforeEach() // do some setup, like say create sinon sandbox 
    afterEach() // restore sandbox
    describe('and there are fewer than the minimum number of characters', () => {
        beforeEach() // set up test string, call validation method 
        it('should indicate the minimum length has not been met')
        it('should allow entry as normal')
    })
    describe('and the input exceeds the character limit', () => {
        beforeEach() // set up test string, call validation method 
        it('should indicate that the maximum length has been exceeded')
        it('should prevent entry of any additional characters')
    })
    describe('and the input is within character limits', () => {
        describe('and contains valid characters', () => {
            beforeEach() // set up test string, call validation method 
            it('should allow entry as normal')
        })
        describe('and contains invalid characters', () => {
            beforeEach() // set up test string, call validation method 
            it('should prevent additional entry')
            it('should indicate that specific characters are invalid')
        })
        describe('and contains swear words', () => {
            beforeEach() // set up test string, call validation method 
            it('should wash your mouth out with soap')
        })
    })
});

This is contrived, but there are often cases where we want to piggy-back on setup and where we want to create readable test output for our specs:

Given input for my overly prescriptive input validator
  and there are fewer than the minimum number of characters
    should indicate the minimum length has not been met
    should allow entry as normal
  and the input exceeds the character limit
    should indicate that the maximum length has been exceeded
    should prevent entry of any additional characters
  and the input is within character limits
    and contains valid characters
      should allow entry as normal
    and contains invalid characters
      should prevent additional entry
      should indicate that specific characters are invalid
    and contains swear words
      should wash your mouth out with soap

I'm sure this isn't a typical practice in the JS community, but it's an example of how far some could be taking their tests.

skawaguchi avatar Apr 12 '17 03:04 skawaguchi

@skawaguchi it seems rather typical to me. Thank you for the examples.

mightyiam avatar Apr 12 '17 04:04 mightyiam

for me as well. and it’s no problem to execute those in parallel. this is nothing but metadata on the tests.

the following conceptually expands to [Test { before }, Test { before }], i.e. two separate tests with a “before” callback attached:

describe({
  beforeEach()
  it()
  it()
})

so if we have this:

describe({
  beforeEach()    // 1
  describe({
    beforeEach()  // 2
    it()
  })
  describe({
    beforeEach()  // 3
    it()
  })
})

it can also expand to a flat list of tests:

[
  Test { [before1, before2] },
  Test { [before1, before3] },
]

flying-sheep avatar Apr 12 '17 07:04 flying-sheep

Has anybody done any work on this issue? If not, I may try implementing it soon to migrate a bunch of existing tests.

thomas4019 avatar Oct 17 '17 18:10 thomas4019

I've stumbled on an approach I'd like to call higher-order-tests. See this example: https://github.com/CDECatapult/ethereum-contract-watcher/blob/0772d3f92f1f9b9413f4ce533423199f4ce9d7cb/test/ContractWatcher.js#L5-L23

The prepare() function comes from a helper which returns the test implementation. It manages the context, performs setup and teardown, and meant I got to write complex tests with little boilerplate.

It'd be good to see more experimentation like this before building in any particular solution to AVA itself.

novemberborn avatar Oct 23 '17 14:10 novemberborn

Worth mentioning how Elixir has approached this in their standard testing library, ExUnit.

ExUnit doesn't allow nested describe blocks and doesn't support the context block. They intentionally designed it this way because they found (based on experience with other testing frameworks) that nesting would lead to very difficult to read and understand tests.

With nested groups you may write your tests as

describe 'Foo'
  describe '.do_something'
    context 'when state is x'
      beforeEach
        set up x

      it 'does primary behaviour'
      it 'does secondary behaviour'

      context 'when state is also y'
        beforeEach
          setup y

          it 'does some behaviour'
          it 'does another behaviour'
  ...

With the limitations on groups in ExUnit, they introduced named setups to easily reuse setup code. So your tests may look like this instead

describe 'Foo.do_something when state is x'
  setup x
  it 'does primary behaviour'
  it 'does secondary behaviour'

describe 'Foo.do_something when state is x and y'
  setup x, y
  it 'does some behaviour'
  it 'does another behaviour'

If you want to find out more about this approach to testing, here's an internal talk I gave on the topic or view just the presentation slides.

dideler avatar Feb 10 '18 18:02 dideler

Thanks for sharing @dideler. Interestingly AVA doesn't even have describe blocks. ExUnit's approach seems similar to my test.fork() suggestion, in that the setup remains explicit despite there not being any actual nesting.

novemberborn avatar Feb 11 '18 15:02 novemberborn

Ava not having describe() blocks or any kind of nesting is actually a big reason I like it.


From this twitter thread: https://twitter.com/jamiebuilds/status/954906997169664000

Before:

describe('foo', () => {
  let foo;
  beforeEach(() => {
    foo = ...
  });

  it('should do something', () => {
    expect(foo.thingImTesting(bar)).to.be.true;
  });

  describe('with bar', () => {
    let bar;
    beforeEach(() => {
      bar = ...
    });

    it('should do something', () => {
      expect(foo.isThisABar(bar)).to.be.true;
    });
  });
});

describe('baz', () => {
  let baz;
  beforeEach(() => {
    baz = ...
  });

  it('should do something', () => {
    expect(baz.hasBeenSetup()).to.be.true;
  });
});

After:

const test = require('ava');

function setupFoo(opts) {...}
function setupBar(opts) {...}
function setupBaz(opts) {...}

test('something that needs foo', t => {
  let foo = setupFoo({...});
  t.is(foo.thingImTesting(), false);
});

test('something that needs foo and bar', t => {
  let foo = setupFoo({...});
  let bar = setupFoo({...});
  t.is(foo.isThisABar(bar), true);
});

test('something that needs bar', t => {
  let baz = setupBaz({...});
  t.is(baz.hasBeenSetup(), true);
});
  1. The setupX methods are written in a way that it can be configured in tests to provide options to the thing you are constructing. In the TDD/BDD way, you'd have to do more refactoring of unrelated tests when adding a new one that wants to be setup differently
  2. The test methods are written so that the explicitly define all of their dependencies, so you aren't setting up extra things just because you'll need them in other tests
  3. The tests can be very easily copied and pasted to create more cases and placed where-ever makes sense in the file without having to copy other bits of code around
  4. Static analysis tools like linters and type checkers can follow the second way much easier because you don't have all these floating values which aren't clearly defined or are temporarily undefined. Ex: A type checker doesn't know what beforeEach means to the variable in tests
  5. This is a bit more minor, but writing tests with a full description in the name (rather than spread out across blocks which might be hundreds of lines apart) is much easier to follow. I hate scrolling up and down just to understand whats going on with my test

jamiebuilds avatar Feb 14 '18 18:02 jamiebuilds

@jamiebuilds I agree with you in common. But your examples seems to be written in favour of non-grouped tests. I think both approaches have own props and cons.

In grouped variant you may use mocha's context this and demonstrate that more tests are added more easily: Before:

describe('foo', () => {
  beforeEach(() => {
    this.foo = setupFoo();
  });

  it('should do something', () => {
    expect(this.foo.thingImTesting(bar)).to.be.true;
  });

  describe('with bar', () => {
    beforeEach(() => {
      this.bar = setupBar();
    });

    it('should do something', () => expect(this.foo.isThisABar(bar)).to.be.true);
    it('should do something else', () => expect(this.foo.isThisABar(bar)).to.be.true);
    it('should do something else else', () => expect(this.foo.isThisABar(bar)).to.be.true);
    // more tests, focused on actual assertion
  });
});

describe('baz', () => {
  beforeEach(() => {
    this.baz = setupBaz();
  });

  it('should do something', () => {
    expect(this.baz.hasBeenSetup()).to.be.true;
  });
});

After:

const test = require('ava');

function setupFoo(opts) {...}
function setupBar(opts) {...}
function setupBaz(opts) {...}

test('something that needs foo', t => {
  let foo = setupFoo({...});
  t.is(foo.thingImTesting(), false);
});

test('something that needs foo and bar', t => {
  let foo = setupFoo({...});
  let bar = setupFoo({...});
  t.is(foo.isThisABar(bar), true);
});

test('something that needs foo and bar else', t => {
  let foo = setupFoo({...}); // duplicated setup
  let bar = setupFoo({...}); // duplicated setup
  t.is(foo.isThisABar(bar), true);
});

test('something that needs foo and bar else else', t => {
  let foo = setupFoo({...}); // duplicated setup
  let bar = setupFoo({...}); // duplicated setup
  t.is(foo.isThisABar(bar), true);
});

test('something that needs bar', t => {
  let baz = setupBaz({...});
  t.is(baz.hasBeenSetup(), true);
});

vitalets avatar Feb 15 '18 13:02 vitalets

@vitalets that doesn't actually work with arrow functions, but this isn't really about syntax. I don't care about the length of code, I care about the functionality and reusability.

As an exercise, look at both examples I gave above and walk through the steps it would take to add a test that needs foo, bar, and baz in each of their test suites.

Here's it without the BDD-style:

test('foo, bar, and baz', t => {
  let foo = setupFoo();
  let bar = setupBar();
  let baz = setupBaz();
  t.is(foo.withBarAndBaz(bar, baz), true);
});

But then BDD-style requires either duplication or refactoring, the easiest way will be to shove everything into a global beforeEach and now you're running all sorts of code which isn't needed in the test.

jamiebuilds avatar Feb 16 '18 03:02 jamiebuilds

I agree with @jamiebuilds on this one. Nested context blocks make tests horribly complex to read. You should always favor extracting common setup into a helper/factory which can be called in each test.

This thoughtbot article gives a very compelling case against nesting context blocks.

danny-andrews avatar Aug 29 '18 18:08 danny-andrews

@jamiebuilds you make great points. I have a few random responses still in support of describe().

I practice TDD and I just can't get the DX I need out of Ava because of the missing describe(). I admit that this could be familiarity bias being used to Mocha and Jest. I keep coming back to check out Ava because neither Mocha or Jest seems to hit the same points on speed that Ava does.

I don't care about the length of code, I care about the functionality and reusability.

I'd like to push back on this. Length of code correlates to readability, which correlates to maintainability. I generally try to balance both code length as well as correctness (functionality) and reusability. None are mutually exclusive, and it's always a trade-off.

Your example code:

test('something that needs foo and bar else else', t => {
  let foo = setupFoo({...}); // duplicated setup
  let bar = setupFoo({...}); // duplicated setup
  t.is(foo.isThisABar(bar), true);
});

This block has great isolation, but at a price. The price is the abstraction of the setup methods, as well as the duplicated code. I could be a little old-school here, but a reasonable amount of DRY has a positive effect. It enhances readability and also reduces the amount of setup code that the reader of your test code has to memorize to know what's going on in a given assertion block. If you combine that with minimizing extraction then I think you end up with easier-to-read test code, which seems to be a central argument against having describe().

Either way, thanks for the suggestions. Even though I don't initially think that they'll give me what I'm looking for, I'll give them a shot.

skawaguchi avatar Nov 27 '19 13:11 skawaguchi

Length of code correlates to readability, which correlates to maintainability.

Length does not correlate to readability or maintainability, or Regex would be known as the most readable and maintainable language ever created.

Code readability is hard to quantify, but there are some traits that definitely correlate to readability much stronger than "length".

Something we can learn from interface design is the value of putting related information physically closer together and visually distinct from unrelated information.

Here's a recent notable example that came from Jira's redesign (GIF is kinda slow, wait for it):

New-Jira-Issue

[Source]

Code benefits from this in the same way, a recent notable example is the redesign of UI logic in React from a class-based structure to "Hooks":

DqsCilOU0AAoS7P

[Source]

BDD-style describe() blocks with state that gets built-up inside beforeEach() blocks leads to the "steps" of the test to be split up and spread all around your test suite.


And for the record, creating setupX() functions is often more DRY than creating beforeEach() blocks, I have done this exact refactor on code bases with thousands of tests, and greatly reduced repetition as a result. For example:

describe("one", () => {
  let a, b, c
  beforeEach(() => {
    a = new A({
      a: "whole",
      bunch: "of",
      code: "to",
      setup: "a",
    })
    b = new B({
      a: "whole",
      bunch: "of",
      code: "to",
      setup: "b",
    })
    c = new C({
      a: "whole",
      bunch: "of",
      code: "to",
      setup: "c",
    })
  })

  test("example 1", () => {
    // ...
  })
})

describe("two", () => {
  let a, b, c
  beforeEach(() => {
    a = new A({
      a: "whole",
      bunch: "of",
      code: "to",
      setup: "a",
      slightly: "different",
    })
    b = new B({
      a: "whole",
      bunch: "of",
      code: "to",
      setup: "b",
      slightly: "different",
    })
    c = new C({
      a: "whole",
      bunch: "of",
      code: "to",
      setup: "c",
      slightly: "different",
    })
  })

  test("example 2", () => {
    // ...
  })
})

describe("three", () => {
  let a, b, c
  beforeEach(() => {
    a = new A({
      a: "whole",
      bunch: "of",
      code: "to",
      setup: "a",
      yet: "another way",
    })
    b = new B({
      a: "whole",
      bunch: "of",
      code: "to",
      setup: "b",
      yet: "another way",
    })
    c = new C({
      a: "whole",
      bunch: "of",
      code: "to",
      setup: "c",
      yet: "another way",
    })
  })

  test("example 3", () => {
    // ...
  })
})

And JavaScript's answer on how to solve this is to pull some of that setup code into reusable functions... just like the setupX() functions I've recommended.

function setupA(opts = {}) {
  return new A({
    a: "whole",
    bunch: "of",
    code: "to",
    setup: "a",
    ...opts,
  })
}

let a1 = setupA()
let a2 = setupA({ slightly: "different" })
let a3 = setupA({ yet: "another way" })

thanos-i-am-inevitable.gif

Also what's this nonsense acting like "DRY" is "old school" lol

jamiebuilds avatar Dec 04 '19 00:12 jamiebuilds

Length does not correlate to readability or maintainability, or Regex would be known as the most readable and maintainable language ever created.

You’re taking my comment out of context. Obviously regex is hard to decipher. Is that really what came across from my point? Not what I intended if so...

BDD-style describe() blocks with state that gets built-up inside beforeEach() blocks leads to the "steps" of the test to be split up and spread all around your test suite.

Agreed, but I’m not asking for beforeEach() everywhere. I’m simply asking for describes to group tests. I generally use a setup method exactly like you’re describing e.g. a factory in React where you can set default props and override them with an argument you pass in.

Your contrived example shows the possibility of redundancy. It has given me pause because that can break isolation, and it’s happened in code bases I’ve worked on. I don’t know if it’s something a linter can catch. Probably, but I’m not positive. It’s worth considering but isn’t a deal breaker because it’s easy to catch.

In terms of your overall argument that UIs can help us understand... cohesion? Yes, I like for my tests to have the related code in them if possible. Yes, I think that having things physically closer together helps us understand that they’re related. But I also like capturing the intention of the test in a describe so I can articulate certain things like the state of the system.

Also what's this nonsense acting like "DRY" is "old school" lol

You forgot the eye-roll emoji 🙄 (I’m joking with you). I’m not sure if your question was serious, but what I mean is that folks seem really caught up in other things like FP or shiny new features in tools. Not many devs have read books like Clean Code and other places where DRY is just something you do, and so I’ve found myself explaining it a lot more than I thought I’d have to. I sort of feel old school when I’m trying to get JS devs to read books in Java in 2008. It sounds like you assume that every developer just knows this stuff. You must be working in a place where that’s common. Lucky you 😉

So anyhow, I’m not sure how my point got turned into a treatise for regex and beforeEach() (although you might be right on the latter). Either way, thanks for taking the time to respond so thoroughly. You made some interesting points, and so I still intend to try out your original suggestion to see if it holds up in more complex test files where I feel like BDD style describes have kept the code organized and understandable.

But I guess the overall point is: describe not welcome in Ava. Got it.

skawaguchi avatar Dec 04 '19 02:12 skawaguchi

But I guess the overall point is: describe not welcome in Ava. Got it.

Actual AVA maintainer here (Jamie's contributions not withstanding) I wouldn't put it that harshly. We're not interested in including alternative test syntax with AVA itself. There's packages like https://www.npmjs.com/package/ava-describe which build on top of AVA.

I am interested in exploring some sort of test-function builders which would give you set-up and tear-down code that is always applied when you use use that test function. I think that could work quite well for setting up scenarios within a single test file, or to share scenarios across test files. And you could use that as a building block for a BDD-style syntax too.

novemberborn avatar Dec 04 '19 09:12 novemberborn

Thanks @novemberborn. I’ll check it out!

skawaguchi avatar Dec 04 '19 13:12 skawaguchi

@novemberborn Please include nest t.context albeit just a way to group tests with a group header ?

tommarien avatar May 27 '21 13:05 tommarien

If ava will ever run in a browser IMHO there groups would be pretty much a requirement, as if you have two asynchronous test suites you need to be able to group their tests somehow to get a clean output for each suite, tests can't be grouped automatically by file name there.

fabiospampinato avatar Jul 23 '21 22:07 fabiospampinato

If a discussion has been debated for 7+ years without a final answer, why not simply implement it and try it out? 🤷‍♂️ maybe it is indeed an anti-pattern, but who cares, it makes me feel happy, if someone thinks it is an anti-pattern, just don't use it, it is better to have a choice than no choice.

ash0080 avatar Feb 10 '22 07:02 ash0080