avajs / ava

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

Implement .because() modifier for irregular tests #3227

Open novemberborn opened 1 year ago

novemberborn commented 1 year ago

Based on discussion with @tommy-mitchell in https://github.com/avajs/ava/discussions/3224:

test.failing('does foo', t => {
  // ..
}).because('waiting on some/repo#123')

Should output:

βœ” [expected fail] does foo
  - waiting on some/repo#123

This requires changes to https://github.com/avajs/ava/blob/main/lib/create-chain.js so that a chainable object is returned for test.failing(), test.skip() etc. Here we then need to add a because function. The reason should make its way to the reporter where it can be printed beneath the test title.

This should be reflected in the type definitions and documentation. Once we've made some progress on this, work needs to be done with https://github.com/avajs/eslint-plugin-ava to support this new modifier.

It should be a test failure if because() is called with anything but a non-empty string.

sindresorhus commented 1 year ago

As commented in #3224, I also think .because() should come last.

novemberborn commented 1 year ago

As commented in #3224, I also think .because() should come last.

That'd be easy to miss with a large test implementation / function body.

It's also a new thing to implement, test() doesn't return anything at the moment, and to make trailers work we'd have to change that, todo, failing, skip, and only. Adding a test.because() function which continues the chain is easier.

tommy-mitchell commented 1 year ago

This could have an ESLint rule as well, requiring a .because() description

tommy-mitchell commented 1 year ago

test() doesn't return anything at the moment, and to make trailers work we'd have to change that, todo, failing, skip, and only

Both #2979 and #2980 propose trailing modifiers on test(), so all three would benefit from it.

novemberborn commented 1 year ago

That's fair β€”Β it's worth exploring but it's a chunk of work. Will amend the issue description.

adiSuper94 commented 1 year ago

@novemberborn If no one has started working on the implementation, I'd like to take a crack at this.

adiSuper94 commented 1 year ago

Turned out to more complex than I anticipated. But I am still working on this.

adiSuper94 commented 1 year ago

@novemberborn I have made some test and trying to test it. But looks like when I run npm run test it calls the old version of create-chain.js instead of the new version that I have made edits to. Is this an issue you have ever faced ??

novemberborn commented 1 year ago

npm test calls npx test-ava which uses a different copy of AVA to test AVA with. But these tests are meant to be integration style, so you'd set up a fixture which uses the new syntax and then you observe the behavior.

adiSuper94 commented 10 months ago

I haven't worked on this for a while now. And I am facing troubles setting up the the test, so that the tests use the my changes. If some else has more experience doing I'd like some help, or they can take up the this issue entirely.

arescrimson commented 9 months ago

@adiSuper94 , I can take up this issue if you want. In addition, what troubles were you having setting up the tests?

oantoro commented 4 weeks ago

I think one of the ways to implement this feature is to let the fn handler to return a new subsequence chain and the callWithFrag() must return it so that the next call to .because() function can be handled. The subsequence handler must be aware of the associated task/test created by the previous handler and also can modify its metadata. Or maybe there is easier approach to solve the problem?

novemberborn commented 1 week ago

@oantoro IIRC that is how the chaining works, the challenge is probably that the "here's the test implementation" is the last step currently, so that needs to change.

oantoro commented 1 week ago

I think additional logic is needed to modify the metadata after the given test is created. So the function callWithFlag() might need some adjustment and also the handler needs some modification to return sub handler where the sub handler will modify the metadata of the associated test to include the error message when the .because(msg) function is called. The idea is to let callWithFlag() to return another chain so that the test.failing(title, cb).because(msg) call can be chained. Also when the task is successfully executed, the message must be emitted here. I think this approach would work but I am not sure whether the additional complexity is worthy or not.