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

Make t.fail() accept an object #2615

Open
dolsem opened this issue Nov 25, 2020 · 5 comments
Open

Make t.fail() accept an object #2615

dolsem opened this issue Nov 25, 2020 · 5 comments

Comments

@dolsem
Copy link

dolsem commented Nov 25, 2020

It seems like the only problem with #1094 is providing detailed error output when a custom assertion fails. Everything else is straightforward. What if t.fail() could accept an object to use for creating an AssertionError? The fields could be message, actual, expected, and format that will specifying which formatting function to use for setting the values field on the error object. Could also add an optional assertion field for a custom assertion name.

Would this be compatible with #2435?

If there's interest, I can submit a PR for this.

@dolsem dolsem changed the title Make t.fail() except an object Make t.fail() accept an object Nov 25, 2020
@novemberborn
Copy link
Member

It's worth exploring, sure.

I'm concerned that properly integrating this with the reporters will be harder than expected. You'd also want to customize the stack trace to not start at the t.fail() call site.

How would you set up custom assertions to use this?

@dolsem
Copy link
Author

dolsem commented Dec 9, 2020

Here's an example of how a custom assertion might be defined:

const test = require('ava');
const { Assertions } = require('ava/lib/assert');

Object.assign(Assertions.prototype, {
  bla(actual, expected) {
    if (!blaTest(actual, expected)) {
      this.fail('error message');
    } else {
      this.pass();
    }
  }  
})

test('bla test', (t) => {
  t.bla(actual, expected);
});

This works but doesn't let us customize the actual error output from AVA. What if we could just modify the fail assertion to pass through an instance of AssertionError to addFailedAssertion()? This way the user will be able to modify the stack appropriately. Or alternatively just save the fail method in a property on the Assertions instance in the constructor, so that people who write custom assertions have access to it?

I haven't seen most of the codebase, but wouldn't reporting work without modifications as long as AssertionError uses the same API?

@novemberborn
Copy link
Member

@dolsem my hunch is that exposes AVA internals more than we'd like. I like an option object better.

@dolsem
Copy link
Author

dolsem commented Dec 14, 2020

@novemberborn well, if you don't want to expose AssertionError, the aforementioned object could have an optional error field with an instance of a custom error. Then AssertionError would reuse its stack trace. It just feels a bit more convoluted than necessary to me. Any particular reason to not expose AssertionError to the outside world? Backwards compatibility considerations? It could be exposed via something like require('ava/internals') as opposed to being available on the main module entrypoint. This way it's more of a "use at your own risk" type of deal.

@novemberborn
Copy link
Member

I want to avoid accidental APIs that we then cannot change. I think there are too many internal considerations for how AssertionError works that are hard to safely expose, hence a "facade" object would be a safer long-term strategy.

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

No branches or pull requests

2 participants