Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support nested groups? #222

Open
novemberborn opened this issue Nov 16, 2015 · 87 comments
Open

Support nested groups? #222

novemberborn opened this issue Nov 16, 2015 · 87 comments
Labels

Comments

@novemberborn
Copy link
Member

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 👍

@vadimdemedes
Copy link
Contributor

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?

@sindresorhus
Copy link
Member

From a quick skim this looks like the same as #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 sindresorhus added enhancement new functionality question labels Nov 16, 2015
@novemberborn
Copy link
Member Author

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.

@sindresorhus
Copy link
Member

@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.

@ArtemGovorov
Copy link
Contributor

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.

@novemberborn
Copy link
Member Author

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.

@sindresorhus
Copy link
Member

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.

@jamestalmage
Copy link
Contributor

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 jamestalmage self-assigned this Nov 30, 2015
@ArtemGovorov
Copy link
Contributor

@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);

@jamestalmage
Copy link
Contributor

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
Copy link
Contributor

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.

@ArtemGovorov
Copy link
Contributor

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.

@jamestalmage
Copy link
Contributor

        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.

@ArtemGovorov
Copy link
Contributor

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.

@jamestalmage
Copy link
Contributor

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).

@ArtemGovorov
Copy link
Contributor

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.

@jamestalmage
Copy link
Contributor

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?

@ArtemGovorov
Copy link
Contributor

@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
Copy link
Contributor

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

@ariporad
Copy link
Contributor

@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.

@matthewbauer
Copy link

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.

@jamestalmage
Copy link
Contributor

Runner.prototype.tests

Well... not on the prototype hopefully. 😄

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

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

@matthewbauer
Copy link

Well... not on the prototype hopefully. 😄

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.

@jamestalmage
Copy link
Contributor

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.

@dideler
Copy link

dideler commented Feb 10, 2018

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.

@novemberborn
Copy link
Member Author

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.

@jamiebuilds
Copy link
Contributor

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

@vitalets
Copy link

@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);
});

@jamiebuilds
Copy link
Contributor

@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.

@danny-andrews
Copy link
Contributor

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.

@skawaguchi
Copy link

@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.

@jamiebuilds
Copy link
Contributor

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

@skawaguchi
Copy link

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.

@novemberborn
Copy link
Member Author

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.

@skawaguchi
Copy link

Thanks @novemberborn. I’ll check it out!

@tommarien
Copy link

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

@fabiospampinato
Copy link

fabiospampinato commented Jul 23, 2021

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.

@ash0080
Copy link

ash0080 commented Feb 10, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests