avajs / ava

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

t.passed is undefined in teardown context #2572

Closed tymfear closed 4 years ago

tymfear commented 4 years ago

Please provide details about:

Code example:

test('do some web testing', async t =>{
  t.teardown(async () => {
    if(!t.passed) {
       const imageName = `${t.title}.png`;
       t.log(`Saving screenshot "${imageName}" because test has failed`);
       await takeScreenshotAndSave(imageName);
    }
  })
})
novemberborn commented 4 years ago

Agreed. We can update t.passed once the test completes, and then it'll be accurate in a teardown.

We run the teardowns here:

https://github.com/avajs/ava/blob/983ce0cb714fe0b35b48834c0911ebd4cf8e71d6/lib/test.js#L728

A little later is the code that determines whether the test passed:

https://github.com/avajs/ava/blob/983ce0cb714fe0b35b48834c0911ebd4cf8e71d6/lib/test.js#L732-L733

We can move that up, and assign this.testPassed = passed. That should do the trick.

Teardown tests live here:

https://github.com/avajs/ava/blob/75cbc3b2a53d3b8530266b10bed71b838bc11fec/test-tap/test.js#L750

tymfear commented 4 years ago

@novemberborn, so I did the following change https://github.com/tymfear/ava/pull/1/files

Tested in manually - it works.

But I have no idea on how to create a test for that. Was thinking of something like

test('t.passed is set in teardown for passing test', t => {
  return ava(a => {
    a.teardown(() => {
      t.is(result.passed, true) // <-------- Don't know how to reach the "passed" value as context is not passed to teardowns
    });
    a.pass();
  }).run().then(result => {
    t.is(result.passed, true);
  });
});

Any idea on this?

Coz the only idea I have is to pass context to teardowns, but that's a huge breaking change (at least it looks to me like it is, not sure). And doing that for testing purpose only seems a bit of overengineering.

But on the other hand, probably it is right way to be consistent with different hooks? And passing context to teardown will remove need for the change I did.

Note: by passing context I mean the t-like object with at least the following values

{
  context,
  passed,
  title
}
tymfear commented 4 years ago

@novemberborn Ok, I found solution on how to create test for the change, but still what are your opinion of passing t-like object to teardowns?

ghost commented 4 years ago

This seems like a pretty interesting issue. But it is already being handled. Kudos to you @tymfear