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

Include raw actual and expected objects in AssertionError #1432

Merged
merged 3 commits into from
Jul 12, 2017

Conversation

ArtemGovorov
Copy link
Contributor

@ArtemGovorov ArtemGovorov commented Jun 30, 2017

We support AVA in wallaby.js and have a few custom renderers for displaying assertions diffs in various editors, like:

VS Code:

screen shot 2017-06-30 at 12 09 25 pm

WebStorm:

screen shot 2017-06-30 at 12 11 50 pm

Wallaby App:

screen shot 2017-06-30 at 12 12 30 pm

Other testing frameworks that we support (also various assertion libs, such as chai) pass raw actual and expected objects with their assertion errors, so we (and other custom reporters) are able to display custom diffs.

AVA's new coloured ANSI diff display is great, but in order to display diffs outside of the ANSI supporting viewer (such as in the cases listed above), it would be great if the AssertionError contained raw actual and expected objects, so that a customer reporter integrated with AVA could use them for doing custom diffs/reporting (the screenshots above show how it'd look like once the PR is landed and published in AVA).

The pull request implements passing the raw actual and expected objects to the AssertionError constructor.

@novemberborn
Copy link
Member

The pull request implements passing the raw actual and expected objects to the AssertionError constructor.

How does Wallaby access these properties? AssertionError is serialized so anything that isn't JSON will get lost as the test results make their way to the main AVA process.

@ArtemGovorov
Copy link
Contributor Author

ArtemGovorov commented Jun 30, 2017

@novemberborn Wallaby doesn't use AVA process. We create an instance of the ava/lib/runner class and use it to run tests in our own process(es). To get the test result we patch addTestResult and get the result.assertError.

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wallaby doesn't use AVA process. We create an instance of the ava/lib/runner class and use it to run tests in our own process(es).

Aha! This should be fine then.

Let's store the values in a AssertionError#raw property (and pass a raw argument to the constructor). It'd help if you could add a code comment saying Wallaby uses this value since it manages AVA worker processes directly, lest we forget and remove it due to being unused.

…operty and pass a raw argument to the constructor
@ArtemGovorov
Copy link
Contributor Author

@novemberborn Great, thanks. Made the requested changes and left the comment (new tests should also help not to remove the property accidentally).

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new tests should also help not to remove the property accidentally

You'd be surprised what I remove, especially if AVA itself doesn't use it 😉


As you're probably aware Wallaby is using internal APIs here. There isn't much overhead in tracking these values so we're happy to help, but it all might still break in the next release.

lib/assert.js Outdated
@@ -29,7 +29,7 @@ function formatWithLabel(label, value) {
}

class AssertionError extends Error {
constructor(opts) {
constructor(opts, raw) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, I meant passing raw as a property in opts.

@ArtemGovorov
Copy link
Contributor Author

ArtemGovorov commented Jul 2, 2017

@novemberborn Fair enough, thanks for your help! Yep, we are aware that we are using AVA's internal APIs and things get broken sometimes, but thankfully our AVA users report them fairly quickly. Made the requested change.

@novemberborn novemberborn merged commit 3f6e134 into avajs:master Jul 12, 2017
@novemberborn
Copy link
Member

Thanks @ArtemGovorov!

kevva pushed a commit that referenced this pull request Sep 13, 2017
This is useful for tools, such as wallaby.js, that bind to AVA's private APIs for tighter integration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants