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

Docs: Really, use t for assertions #1031

Closed
danprince opened this issue Sep 2, 2016 · 8 comments
Closed

Docs: Really, use t for assertions #1031

danprince opened this issue Sep 2, 2016 · 8 comments

Comments

@danprince
Copy link

I just tried to covert a project that's tested with tape over to using AVA instead. The tests all follow this kind of format:

test('function-a', (assert) => {
  assert.plan(1);

  assert.is(
    functionA(),
    resultB,
    'function a should return result b'
  );
});

They seem to run just fine under AVA, but the assertion messages don't show up when I create a failing test case, just the test messages.

Is there a way to report on the assertion messages? Or is the best practice just to wrap every one of these assertions in its own test block? (In which case, why do the assertions accept optional message parameters?

Am I missing something stupid?

@jamestalmage
Copy link
Contributor

jamestalmage commented Sep 2, 2016

AVA doesn't report assertion messages unless one fails. It only reports test failure / success.

Changing the behaviour to be more like tape would be problematic because so many tests run and report concurrently. The onslaught of output would be very hard to decipher.

@danprince
Copy link
Author

Thanks @jamestalmage. Looking at the AVA branch again, I realise I slightly misreported the problem I was having.

When a test fails I get the message, but none of the details I'd need to actually start fixing it. Seems like this might just be the behaviour for the default reporter?

I tried the tap reporter to check and it does report the failing conditions, but it seems like it uses .toString to show the actual and expected values.

For comparison here's what I would see from the same tests run under tape without a reporter.

Is there a way to enable this kind of reporting with either the default reporter, or the tap one?

@jamestalmage
Copy link
Contributor

Try using t instead of assert as your parameter name (using t should enable power-assert).

Also, t.is does strict comparison, not deepEqual. Use t.deepEqual for that.

@danprince
Copy link
Author

danprince commented Sep 2, 2016

Try using t instead of assert as your parameter name (using t should enable power-assert).

Really?

It seems to work, but that's kinda voodoo.

All the same, if possible I'd prefer to stick with tap-spec and the output produced with the --tap flag still shows [object Object] even using t rather than assert.

Happy to make the PR if this just needs a more thought out string converter for the tap reporter.

@jamestalmage
Copy link
Contributor

Improved TAP output would certainly be welcome. I'd like to see someone pick up where I left off in tap-emitter and land a PR that switches to using that.

Happy to make the PR if this just needs a more thought out string converter for the tap reporter.

It's a little more complicated than that. It involves YAML conversion, among other things. Right now, our TAP output is very naive. The TAP spec actually has some complex rules that we aren't following very well, but tap-emitter aims to fix that.

The full TAP spec that we are trying to follow is defined in this PR. Despite it's draft status, it represents the "reality on the ground" for the tap output of most Node.js test frameworks, and it's definitely what tap-spec and most popular Node reporters expect. Specifically, it introduces some additional constraints around serializing objects into YAML (which is directly related to your issue).

Unfortunately, that draft spec has languished for a year now, so it's claim to represent "reality on the ground" is likely out of date in some way. Still, I recommend starting with that spec.

If you're serious about contributing, take a look at avajs/tap-emitter#1 first. It would be a great way to familiarize yourself with the spec, and push things along.

@jamestalmage
Copy link
Contributor

Really?

It seems to work, but that's kinda voodoo.

Yes, yes it is. This is a byproduct of the way power-assert works. It uses a pattern matching scheme that makes it easier for implementors to wrap any assertion library with power-assert goodness without having to understand the ES AST at all (which is super awesome). That said, the AVA team certainly has members capable of creating a more robust matcher that would detect your use of a different variable name, so if power-assert exposed an "advanced" API, we could certainly leverage it to provide a more user friendly experience.

See power-assert-js/babel-plugin-espower#18.

It would be non-trivial, but something worth doing.

@smeijer
Copy link

smeijer commented Sep 5, 2016

Try using t instead of assert as your parameter name (using t should enable power-assert).

I had this exact same issue. I guess it's worth a note in the readme, perhaps under FAQ.

Why is the power-assert information not shown?

Try using t instead as your parameter name.

This is a byproduct of the way power-assert works. It uses a pattern matching scheme that makes it easier for implementors to wrap any assertion library with power-assert goodness without having to understand the ES AST at all

JaKXz added a commit to JaKXz/glisp that referenced this issue Sep 5, 2016
@novemberborn
Copy link
Member

Yes the README should be clearer that you should really use t. We should add it to https://github.com/avajs/ava/blob/master/docs/common-pitfalls.md as well.

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

4 participants