Magic assert #1154

vadimdemedes · 2016-12-12T21:00:20Z

Todo:

sindresorhus · 2016-12-17T00:52:48Z

lib/code-excerpt.js

+const chalk = require('chalk');
+
+function formatLineNumber(line, maxLines) {
+	return repeating(' ', String(maxLines).length - String(line).length) + line;


You can use String#repeat() now.

sindresorhus · 2016-12-17T00:56:11Z

package.json

    "pretty-ms": "^2.0.0",
+    "react": "^15.4.0",


Ah, need to check that.

React is needed by react-element-to-jsx-string module. I could try submitting a PR (if possible) to avoid the dependency on React.

sindresorhus · 2016-12-17T01:03:39Z

Can you comment on what's left?

sindresorhus · 2016-12-17T01:05:33Z

I noticed $ ava and $ ava --verbose has some inconsistent output. We should really try to unify the output and share logic between them. The output below 1 failed and 1 test failed [02:03:57] should be identical:

sindresorhus · 2016-12-17T01:07:31Z

For t.fail(), I think we should make an exception and not show the Actual/Expected output. It makes no sense and the code excerpt is clear enough.

cpojer · 2016-12-17T06:16:52Z

package.json

-    "power-assert-context-formatter": "^1.0.4",
-    "power-assert-renderer-assertion": "^1.0.1",
-    "power-assert-renderer-succinct": "^1.0.1",
+    "pretty-format": "github:vadimdemedes/pretty-format#colors",


fyi pretty-format 18 is out :)

This is a fork with support for colorizing the output, from what I can see the latest release doesn't have smth similar.

@vadimdemedes What's the intention here? Upstream those changes? We cannot depend on a git branch, so we'll either have to upstream or you need to publish your fork to npm.

If we're forking, we could publish to @ava/pretty-format.

I will publish it as @ava/pretty-format and resubmit my syntax highlighting proposal to facebook/jest (original at https://github.com/thejameskyle/pretty-format/issues/57).

Will move my fork over to our org too.

How I did not know about pretty-format. :-(

Seems like I reinvented the wheel https://www.npmjs.com/package/prettyprint.

Haha yeah, it took me a while to find out about it too.

Need to remove this line.

novemberborn · 2016-12-19T13:28:30Z

lib/format-assert-error.js

+		`${indentString(err.actual, 2)}\n`,
+		'Expected:\n',
+		`${indentString(err.expected, 2)}\n`
+	].join('\n');


Could use a single template string.

Tried that, but didn't work out, because the resulting string contains code's indentation:

Actual: ... Expected: ...

Am I doing smth wrong here?

No, that's a common issue when using template strings like that. You either need to just remove the indent, which looks weird, or use something like https://github.com/sindresorhus/redent In this case, I don't think it's worth it.

novemberborn · 2016-12-19T13:31:09Z

lib/format-assert-error.js

+		const patch = diff.createPatch('string', err.actual, err.expected);
+		const msg = patch
+			.split('\n')
+			.splice(4)


Should use .slice(4).

novemberborn · 2016-12-19T13:32:28Z

lib/cli.js

@@ -112,7 +114,7 @@ exports.run = function () {
 		explicitTitles: cli.flags.watch,
 		match: arrify(cli.flags.match),
 		babelConfig: babelConfig.validate(conf.babel),
-		resolveTestsFrom: cli.input.length === 0 ? pkgDir : process.cwd(),
+		resolveTestsFrom: resolveTestsFrom,


Can just specify resolveTestsFrom, if you like.

vadimdemedes · 2016-12-27T19:36:31Z

Can you comment on what's left?

From what I can remember, mostly tests. I also wanted to check if t.jsxEquals works equally great with Preact.

The output below 1 failed and 1 test failed [02:03:57] should be identical

I think this can be solved right now outside of this PR, as this change would be unrelated.

For t.fail(), I think we should make an exception and not show the Actual/Expected output. It makes no sense and the code excerpt is clear enough.

Sure, good catch.

vadimdemedes · 2017-01-05T14:38:31Z

Added a todo list, so that you guys know what's going on. I squash commits right away, so there wasn't a quick way to track progress.

sindresorhus · 2017-01-10T03:43:01Z

I squash commits right away, so there wasn't a quick way to track progress.

Can you not squash? It makes it impossible to review what changed. Just add commits and we'll squash at the end when merging.

vadimdemedes · 2017-01-10T07:29:46Z

Good point.

sindresorhus · 2017-01-11T19:52:11Z

@vadimdemedes Note this issue when using this PR: #1176 (I've confirmed it)

sindresorhus · 2017-01-16T10:23:16Z

lib/assert.js

+	test(treeEquals, create(val, expected, '===', msg, x.jsxEqual));
+};
+
+x.jsxNotEqual = (val, expected, msg) => {


Shouldn't this be notJsxEqual?

I was changing it back and forth, probably should be that way indeed. I didn't like capitalization of jsx is different in jsxEqual and notJsxEqual.

I agree about it looking uglier, but better to be consistent and always have not first. In a perfect world we would use snakecase in JS...

vadimdemedes · 2017-01-17T20:56:59Z

What output should we display for these tests? https://github.com/avajs/ava/blob/master/test/fixture/throw-named-function.js https://github.com/avajs/ava/blob/master/test/fixture/throw-anonymous-function.js

sindresorhus · 2017-01-18T07:48:28Z

@vadimdemedes For reference, currently they output:

❯ ava

  1 passed
  1 exception

  Uncaught Exception
  function fooFn() {}

❯ ava

  1 passed
  1 exception

  Uncaught Exception
  () => {}

I don't think we can do much better than that, as we have no information when people throw non-errors. We could maybe better indicate that it's a non-error. For example:

❯ ava

  1 passed
  1 exception

  Uncaught Exception
  Threw non-error: function fooFn() {}

mightyiam · 2017-01-18T11:07:55Z

"Threw non-error" seems sweet.

vadimdemedes · 2017-01-30T21:25:10Z

Attention everyone, magic-assert just crossed off all points from the todo list and is very short from landing in master! I would very appreciate, if you tested it out in some real-world projects and report any issues here. Thank you!

jfmengels · 2017-01-30T21:58:56Z

The error output is a bit odd when using t.throws.

There is a comparison of actual and expected, which may make sense when comparing the thrown errors, but here there is one missing. I think it would be clearer by not having this comparison
Missing expected exception has two dots at the end. I feel like this one has always been like this, so maybe for another PR. That said, I never found this error very comprehensive: Does t.throws expect me to pass an exception type/message, or did it run and the function simply did not throw? Happy to create a different issue to discuss this.

Edit: pretty much the same thing for notThrows when you have an error. Comparing does not make much sense, and the two dots are there too.

I don't think this is blocking, this could be improved upon in a later PR/version.

vadimdemedes · 2017-01-31T06:45:07Z

@jfmengels Great catch, I'll disable assertion output for t.throws. And yes, dots were there before, will create a beginner-friendly issue.

novemberborn

Good stuff @vadimdemedes!

novemberborn · 2017-01-31T11:41:41Z

lib/assert.js

-	const result = toMatchSnapshot.call(context, tree);
+	// symbols can't be serialized and saved in a snapshot,
+	// that's why tree is saved in `jsx` prop, so that JSX can be detected later
+	const serializedTree = tree.$$typeof === Symbol.for('react.test.json') ? {jsx: tree} : tree;


Can we make the jsx key a bit more obscure? E.g. __ava_jsx. Given the deserialization if your tree happens to include a top-level jsx key you're bound to get odd results.

novemberborn · 2017-01-31T12:11:58Z

package.json

-    "power-assert-context-formatter": "^1.0.4",
-    "power-assert-renderer-assertion": "^1.0.1",
-    "power-assert-renderer-succinct": "^1.0.1",
+    "pretty-format": "github:vadimdemedes/pretty-format#colors",


Need to remove this line.

vadimdemedes · 2017-01-31T22:05:09Z

@novemberborn @jfmengels PR updated, thanks! No more output for t.throws/t.doesNotThrow.

vadimdemedes · 2017-02-01T11:15:01Z

Added a new todo:

Fix code excerpt extraction from non-test files when babel-register is involved (line number doesn't match the actual one)

novemberborn · 2017-02-01T14:09:37Z

Fix code excerpt extraction from non-test files when babel-register is involved (line number doesn't match the actual one)

Is this a source maps issue? I've always assumed that babel-core/register causes conflicts in source-map-support. We'll be rolling our own solution with RFC 001 so perhaps this isn't worth worrying about.

sindresorhus · 2017-02-01T15:32:35Z

Is this a source maps issue? I've always assumed that babel-core/register causes conflicts in source-map-support. We'll be rolling our own solution with RFC 001 so perhaps this isn't worth worrying about.

@vadimdemedes Can you open an issue about this and mention that this will be handled by RFC1?

vadimdemedes · 2017-02-01T17:35:45Z

Is this a source maps issue?

@novemberborn Yes, I think so, because it points to a compiled file, not the original.

Cool, I'll open an issue, so that we don't forget about this. Until then, I'll add a change, that prevents AVA from crashing, when there's no code excerpt available.

vadimdemedes · 2017-02-01T22:14:34Z

Done, AVA doesn't crash anymore when code excerpt points to the wrong location and is broken.

sindresorhus · 2017-02-02T04:51:50Z

This is absolutely amazing @vadimdemedes! So good in fact, I almost want test failures now 😝

vadimdemedes · 2017-02-02T06:05:01Z

Thank you, thank you, thank you! I'm sooo excited! Can't wait for the first bug report :D

vadimdemedes force-pushed the magic-assert branch from e4bd045 to 57af416 Compare December 12, 2016 21:00

vadimdemedes mentioned this pull request Dec 12, 2016

Magic assert #1155

Closed

sindresorhus requested changes Dec 17, 2016

View reviewed changes

cpojer reviewed Dec 17, 2016

View reviewed changes

novemberborn requested changes Dec 19, 2016

View reviewed changes

vadimdemedes force-pushed the magic-assert branch from 57af416 to ff62d97 Compare December 27, 2016 19:53

vadimdemedes force-pushed the magic-assert branch 6 times, most recently from 8a4c3ca to 6cd9799 Compare January 5, 2017 14:34

vadimdemedes force-pushed the magic-assert branch 3 times, most recently from 776b37d to ca94ba5 Compare January 9, 2017 22:52

sindresorhus reviewed Jan 16, 2017

View reviewed changes

sindresorhus mentioned this pull request Jan 19, 2017

Better string and object diffs #406

Closed

vadimdemedes force-pushed the magic-assert branch 2 times, most recently from ca9eb64 to 38ec942 Compare January 19, 2017 19:52

vadimdemedes force-pushed the magic-assert branch from f16e16f to 9e9642f Compare January 30, 2017 20:02

Vadim Demedes added 4 commits January 30, 2017 22:20

remove react element rendering

2508272

use babel-preset-transform-test-files@2

efe21ac

use @ava/pretty-format

62a25a7

cleanup

286d620

novemberborn requested changes Jan 31, 2017

View reviewed changes

vadimdemedes force-pushed the magic-assert branch from 0c107fd to 198ad2e Compare January 31, 2017 22:05

Vadim Demedes added 3 commits February 1, 2017 00:07

remove previous pretty-format

cf6ee8e

change custom jsx key in snapshots

b139bc6

disable assertion output for t.throws

9ea5af4

vadimdemedes force-pushed the magic-assert branch from 198ad2e to 9ea5af4 Compare January 31, 2017 22:07

novemberborn approved these changes Feb 1, 2017

View reviewed changes

skip code excerpt when its broken

443888f

vadimdemedes mentioned this pull request Feb 1, 2017

Error stack points to compiled source when using babel-register #1217

Closed

sindresorhus approved these changes Feb 2, 2017

View reviewed changes

sindresorhus merged commit c9e6e6f into master Feb 2, 2017

sindresorhus deleted the magic-assert branch February 2, 2017 04:51

novemberborn mentioned this pull request Feb 17, 2017

Snapshot recap #1275

Closed

novemberborn mentioned this pull request Mar 20, 2017

Include anonymous functions (whose name node cannot infer) in stack traces #1313

Closed

Magic assert #1154

Magic assert #1154

Conversation

vadimdemedes commented Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Dec 17, 2016

sindresorhus commented Dec 17, 2016

sindresorhus commented Dec 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes commented Dec 27, 2016

vadimdemedes commented Jan 5, 2017

sindresorhus commented Jan 10, 2017

vadimdemedes commented Jan 10, 2017

sindresorhus commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes commented Jan 17, 2017

sindresorhus commented Jan 18, 2017

mightyiam commented Jan 18, 2017

vadimdemedes commented Jan 30, 2017

jfmengels commented Jan 30, 2017 • edited Loading

vadimdemedes commented Jan 31, 2017

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes commented Jan 31, 2017

vadimdemedes commented Feb 1, 2017

novemberborn commented Feb 1, 2017

sindresorhus commented Feb 1, 2017

vadimdemedes commented Feb 1, 2017

vadimdemedes commented Feb 1, 2017

sindresorhus commented Feb 2, 2017

vadimdemedes commented Feb 2, 2017 • edited Loading

vadimdemedes commented Dec 12, 2016 •

edited

Loading

vadimdemedes Jan 9, 2017 •

edited

Loading

jfmengels commented Jan 30, 2017 •

edited

Loading

vadimdemedes commented Feb 2, 2017 •

edited

Loading