avajs / ava

Node.js test runner that lets you develop with confidence πŸš€
MIT License
20.74k stars 1.41k forks source link

Setup Functions (t.teardown) #1873

Closed jamiebuilds closed 4 years ago

jamiebuilds commented 6 years ago

Issuehunt badges

This is going to be a bit of a more testing-philosophy proposal. I'm so sorry for writing such a long issue, but it's mostly code examples of the same test suite so hopefully it won't take very long to get through.

TL;DR

It's very hard to write setup and teardown (beforeEach/afterEach) functions "properly" in pretty much every test framework that exists today. To solve this, what if AVA had "Setup Functions":

function createComponent(t, opts) {
  let component = new Component(opts)
  t.teardown(() => component.destroy())
  return component
}

test("component when foo does this thing", t => {
  let component = createComponent(t, { option: true })
  // ...
})

The Problem: Setting stuff up for your tests

BDD-style

For a while now, the JavaScript community has been writing "BDD-style" tests like:

describe("Component", () => {
  let component

  beforeEach(() => { 
    component = new Component()
  })

  describe("when foo", () => {
    let foo;

    beforeEach(() => { 
      foo = new Foo()
    })

    it("should do this thing", () => {
      expect(component.doThisThing(foo)).toBe(true)
    })
  })

  describe("when bar", () => {
    let bar

    beforeEach(() => { 
      bar = new Bar()
    })

    it("should do this thing", () => {
      expect(component.doThisThing(bar)).toBe(true)
    })

    describe("and baz", () => {
      let baz

      beforeEach(() => { 
        baz = new Baz()
      })

      it("should do this thing", () => {
        expect(component.doThisThing(bar, baz)).toBe(true)
      })
    })
  })
})

This has some notable problems:

AVA-style

In contrast, this is one way you could write that same test suite in AVA:

import test from "ava"

test.beforeEach(t => { 
  t.context.component = new Component()
  t.context.foo = new Foo()
  t.context.bar = new Bar()
})

test("component when foo does this thing", t => {
  t.true(t.context.component.doThisThing(foo))
})

test("component when bar does this thing", t => {
  t.true(t.context.component.doThisThing(t.context.bar))
})

test("component when bar and baz does this thing", t => {
  t.true(t.context.component.doThisThing(t.context.bar, t.context.baz))
})

There are still problems here though:

AVA+setup functions-style

So instead, you'd probably write it like this:

let createComponent = () => new Component()
let createFoo = () => new Foo()
let createBar = () => new Bar()

test("component when foo does this thing", t => {
  let component = createComponent()
  let foo = createFoo()
  t.true(component.doThisThing(foo))
})

test("component when bar does this thing", t => {
  let component = createComponent()
  let bar = createBar()
  t.true(component.doThisThing(bar))
})

test("component when bar and baz does this thing", t => {
  let component = createComponent()
  let bar = createBar()
  let baz = createBar()
  t.true(component.doThisThing(bar, baz))
})

This works great because:

Tearing setup functions down

However, there's still a problem:

With test.beforeEach this is easy:

test.always.afterEach(t => {
  t.context.component.destroy()
  t.context.foo.destroy()
  t.context.bar.destroy()
})

But with setup functions, it gets trickier. You end up writing something like this:

let _component
let _foo
let _bar

function createComponent() {
  let _component = new Component()
  return _component
}

function createFoo() {
  let _foo = new Foo()
  return _foo
}

function createBar() {
  let _bar = new Bar()
  return _bar
}

test.always.afterEach(() => {
  _component.destroy()
  _foo.destroy()
  _bar.destroy()
})

// ...

Tearing setup functions down - Part 2

Except... that doesn't work:

Instead you need to write it like:

function createComponent(t) {
  t.context.component = new Component()
  return t.context.component
}

function createFoo() {
  t.context.foo = new Component()
  return t.context.foo
}

function createBar() {
  t.context.bar = new Component()
  return t.context.foo
}

test.always.afterEach(t => {
  if (t.context.component) t.context.component.destroy()
  if (t.context.foo) t.context.foo.destroy()
  if (t.context.bar) t.context.bar.destroy()
})

test("component when foo does this thing", t => {
  let component = createComponent(t)
  let foo = createFoo(t)
  t.true(component.doThisThing(foo))
})

// ...

This is a lot to have to remember, so I want to propose an alternative:

The (Proposed) Solution: t.teardown()

What if we instead tried placing the teardown code within the setup functions?

import test from "ava"

function createComponent(t) {
  let component = new Component()
  t.teardown(() => component.destroy())
  return component
}

function createFoo(t) {
  let foo = new Foo()
  t.teardown(() => foo.destroy())
  return foo
}

function createBar(t) {
  let bar = new Bar()
  t.teardown(() => bar.destroy())
  return bar
}

test("component when foo does this thing", t => {
  let component = createComponent(t)
  let foo = createFoo(t)
  t.true(component.doThisThing(foo))
})

test("component when bar does this thing", t => {
  let component = createComponent(t)
  let bar = createBar(t)
  t.true(component.doThisThing(bar))
})

test("component when bar and baz does this thing", t => {
  let component = createComponent(t)
  let bar = createBar(t)
  let baz = createBar(t)
  t.true(component.doThisThing(bar, baz))
})

t.teardown(cb) would:

This solves the entire problem space:

I would like to see AVA implement something like this and promote it within the docs.

Open Questions


IssueHunt Summary #### [ulken ulken](https://issuehunt.io/u/ulken) has been rewarded. ### Backers (Total: $80.00) - [issuehunt issuehunt](https://issuehunt.io/u/issuehunt) ($80.00) ### Submitted pull Requests - [#2459 Add `t.teardown()`](https://issuehunt.io/r/avajs/ava/pull/2459) --- ### Tips - Checkout the [Issuehunt explorer](https://issuehunt.io/r/avajs/ava/) to discover more funded issues. - Need some help from other developers? [Add your repositories](https://issuehunt.io/r/new) on IssueHunt to raise funds.
novemberborn commented 6 years ago

That's a neat idea @jamiebuilds.

This could be done using a try {} finally {} block inside the test implementation, but that's not very pretty. I'd be OK with adding a teardown util.

I think the callbacks should execute within the test, but orchestrated by AVA. They have access to the t object through the closure so they could still perform assertions.

Presumably callbacks are executed in order, serially, waiting for any returned promises to settle? Errors wouldn't stop the next callback from executing?

chrisdarroch commented 6 years ago

If an error gets thrown during teardown it’s likely you want to fail the test. Teardown is theoretically cleaning up side-effects to put the suite back in to a known state. an error during cleanup is a warning that subsequent tests will run in an unknown state. if you ignore those, subsequent test failures may be cryptic.

jamiebuilds commented 6 years ago

Presumably callbacks are executed in order, serially, waiting for any returned promises to settle? Errors wouldn't stop the next callback from executing?

So I have two opposing thoughts:

if you ignore those, subsequent test failures may be cryptic.

I don't think you have to ignore them, I think it would be possible to report an error in teardown separately from the test itself passing. The test itself could also be failing and you'd still want the teardown code to run, which could then also fail (likely for the same reason as the test failure), and you'd want both errors to be reported.

That being said I can see arguments for both choices.

novemberborn commented 6 years ago

The before & after hooks run concurrently by default, are invoked in order, and can be made to run serially.

I don't want to add t.teardown.serial(), so the question is what the default behavior should be. I propose we make them execute in order and serially. You can always compose different behavior in an initial teardown callback, if needed for performance reasons.

Before & after hooks can execute their own assertions, as they receive their own t value. I reckon the callback will be a closure with access to the t value of the test. We could make it crash if it performs an assertion on that object, but that seems like a pitfall. Providing its own t value just leads to shadowing:

test('test', t => {
  t.teardown(t => {
    // Which value is `t`?
  })
})

That doesn't seem ideal either. However if the teardown can perform assertions on the test, then I don't think we should report teardown failures separately.

Tests keep executing even if there is an assertion failure. We can do the same for teardown callbacks. There's an open issue for AVA to report multiple assertion failures though.

jamiebuilds commented 6 years ago

I propose we make them execute in order and serially

Okay, that sounds good πŸ‘

test('test', t => {
  t.teardown(t => {
    // Which value is `t`?
  })
})

It seems a bit unnecessary to provide t.teardown's callback with t. Any scope where you can call t.teardown you should already have access to t.

I also want this to work without any weirdness:

t.teardown(db.close);
// Does db.close receive an unexpected `t` argument? What if that changed its behavior

However if the teardown can perform assertions on the test, then I don't think we should report teardown failures separately.

That's fair πŸ‘

havenchyk commented 6 years ago

@jamiebuilds are you still interested in implementing this? πŸ˜„

jamiebuilds commented 6 years ago

I had started on an implementation somewhere but if you're wanting to grab it feel free

havenchyk commented 6 years ago

No-no, it will take ages for me to implement this @jamiebuilds

I just found this idea very cool and useful and I wanted to check was there any activity after the last comment at this issue

btkostner commented 5 years ago

While not exactly the same, I noticed in the 1.0 release docs that you can now do helper setup functions. https://github.com/avajs/ava/blob/master/docs/recipes/puppeteer.md

IssueHuntBot commented 5 years ago

@issuehunt has funded $80.00 to this issue.


tryzniak commented 5 years ago

Can't this be solved by using helper functions and variations?

async function withComponent(t, run) {
  const component = {
    name: 'Dropdown',
    alive: true,
    destroy() {
      this.alive = false
    }
  }

  await run(t, component)
  component.destroy();
}

test("component", withComponent, async (t, comp) => {
  t.is(comp.name, 'Dropdown')
  t.true(comp.alive)
})

Unfortunately this feature has not be documented well, other than a mention here.

novemberborn commented 5 years ago

Yes but it's hard to get that right. Like, you need a try / finally to ensure the component is destroyed if run() throws. But you probably don't want the error from component.destroy() to replace the one from run()… So a teardown utility may still be useful.

issuehunt-oss[bot] commented 4 years ago

@novemberborn has rewarded $72.00 to @ulken. See it on IssueHunt