-
-
Notifications
You must be signed in to change notification settings - Fork 304
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
[Breaking] error
should not emit expected
/actual
diags
#455
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a potential breaking change. Is the current output causing problems for you with a downstream tap reporter?
@@ -370,7 +370,7 @@ function error(err, msg, extra) { | |||
this._assert(!err, { | |||
message : defined(msg, String(err)), | |||
operator : 'error', | |||
actual : err, | |||
error : err, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this field being read?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive me, that was imprecise. What I should have said was that field is read off of the opts
object here:
https://github.com/substack/tape/blob/a1e8f7e21cc7831cf5a857b00627b5dbcdd68aea/lib/test.js#L224
and that line populates the error
field on the 'result'
event, which itself is read in the location I linked above.
@@ -37,10 +35,6 @@ tap.test('preserves stack trace with newlines', function (tt) { | |||
+ 'not ok 1 Error: Preserve stack\n' | |||
+ ' ---\n' | |||
+ ' operator: error\n' | |||
+ ' expected: |-\n' | |||
+ ' undefined\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally i do find this useful - it makes it clear what value is expected to make it pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, there isn't really an expected value, though... I think of error
as like fail
, where you call it to report that something went wrong, and the expected behavior is that it not get called at all. expected: undefined
doesn't add any additional information on top of operator: error
. If you find it useful anyway, hey, you do you, but I don't really understand why.
Hi, thanks for getting back to me so quickly! I've tested this with both Here is a quick and dirty const tap = require(process.argv[2]);
tap.test('example test producing error', t => {
t.error(new Error('this is a message'));
t.end();
});
tap.test('example test producing unexpected result', t => {
t.equals('bad', 'good');
t.end();
});
In both cases, the message of the error is both reported as the reason for the test failure and diffed against the text ‘undefined’ (tap-difflet's handling of this looks especially bad). I think the message alone is sufficient; after all, I figure that For comparison, here's how node-tap fares:
( |
error
should not emit expected
/actual
diagserror
should not emit expected
/actual
diags
Thanks! |
Changes since v5.0.0-next.0: - [Breaking] fail any assertion after `.end()` is called (#489( - [Breaking] tests with no callback are failed TODO tests (#69) - [Breaking] equality functions: throw when < 2 arguments are provided - [Breaking] add "exports" to restrict public API - [Breaking] `throws`: bring into line with node’s `assert.throws` - [Breaking] use default `require.extensions` collection instead of the magic Array `['.js']` (#396) - [Fix] error stack file path can contain parens/spaces - [Refactor] make everything strict mode - [Refactor] Avoid setting message property on primitives; use strict mode to catch this (#490) - [Refactor] generalize error message from calling `.end` more than once - [Dev Deps] update `eslint` - [Tests] improve some failure output by adding messages - [Tests] handle stack trace variation in node <= 0.8 - [Tests] ensure bin/tape is linted - [Tests] Fail a test if its callback returns a promise that rejects (#441) - [eslint] fix remaining undeclared variables (#488) - [eslint] Fix leaking variable in tests - [eslint] fix object key spacing Changes since v4.12.1: - [Breaking] `error` should not emit `expected`/`actual` diags (#455) - [Breaking] support passing in an async function for the test callback (#472) - [Breaking] update `deep-equal` to v2 - [Deps] update `resolve` - [meta] change dep semver prefix from ~ to ^
Changes since v5.0.0-next.3: - [Fix] `.catch` is a syntax error in older browsers - [Refactor] remove unused code - [Deps] update `resolve` Changes since v4.13.0: - [Breaking] update `deep-equal` to v2 - [Breaking] fail any assertion after `.end()` is called (#489) - [Breaking] `error` should not emit `expected`/`actual` diags (#455) - [Breaking] support passing in an async function for the test callback (#472) - [Breaking] tests with no callback are failed TODO tests (#69) - [Breaking] equality functions: throw when < 2 arguments are provided - [Breaking] add "exports" to restrict public API - [Breaking] `throws`: bring into line with node’s `assert.throws` - [Breaking] use default `require.extensions` collection instead of the magic Array `['.js']` (#396) - [meta] change dep semver prefix from ~ to ^
Changes since v5.0.0-next.4: - [Breaking] only `looseEqual`/`deepEqual, and their inverses, are now non-strict. - [Breaking] make equality functions consistent: - [Breaking] `equal`: use `==`, not `===`, to match `assert.equal` - [Breaking] `strictEqual`: bring `-0`/`0`, and `NaN` into line with `assert` - [patch] Print name of test that didnt end (#498) - [Refactor] remove unused code - [Deps] update `resolve` Changes since v4.13.2: - [Breaking] only `looseEqual`/`deepEqual, and their inverses, are now non-strict. - [Breaking] make equality functions consistent: - [Breaking] `equal`: use `==`, not `===`, to match `assert.equal` - [Breaking] `strictEqual`: bring `-0`/`0`, and `NaN` into line with `assert` - [Breaking] update `deep-equal` to v2 - [Breaking] fail any assertion after `.end()` is called (#489) - [Breaking] `error` should not emit `expected`/`actual` diags (#455) - [Breaking] support passing in an async function for the test callback (#472) - [Breaking] tests with no callback are failed TODO tests (#69) - [Breaking] equality functions: throw when < 2 arguments are provided - [Breaking] add "exports" to restrict public API - [Breaking] `throws`: bring into line with node’s `assert.throws` - [Breaking] use default `require.extensions` collection instead of the magic Array `['.js']` (#396) - [meta] change dep semver prefix from ~ to ^
Unlike
throws
, where one can argue that the error thrown is anactual
that should be compared to anexpected
error provided tothrows
, the error provided toerror
is not compared to anything. It should therefore not appear in anexpected
/actual
pair, where downstream TAP reporters can pick it up and diff it againstundefined
—I can't think of a case where such output would be helpful.