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

[Fix] `error` should not emit `expected`/`actual` diags #455

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@rhendric
Copy link

rhendric commented Jan 6, 2019

Unlike throws, where one can argue that the error thrown is an actual that should be compared to an expected error provided to throws, the error provided to error is not compared to anything. It should therefore not appear in an expected/actual pair, where downstream TAP reporters can pick it up and diff it against undefined—I can't think of a case where such output would be helpful.

@ljharb
Copy link
Collaborator

ljharb left a comment

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,

This comment has been minimized.

@ljharb

ljharb Jan 6, 2019

Collaborator

where is this field being read?

This comment has been minimized.

@rhendric

rhendric Jan 7, 2019

Here, I believe:

var errorStack = res.error && res.error.stack;

This comment has been minimized.

@rhendric

rhendric Jan 8, 2019

Forgive me, that was imprecise. What I should have said was that field is read off of the opts object here:

res.error = defined(extra.error, opts.error, new Error(res.name));

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'

This comment has been minimized.

@ljharb

ljharb Jan 6, 2019

Collaborator

Personally i do find this useful - it makes it clear what value is expected to make it pass.

This comment has been minimized.

@rhendric

rhendric Jan 7, 2019

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.

@rhendric

This comment has been minimized.

Copy link

rhendric commented Jan 7, 2019

Hi, thanks for getting back to me so quickly!

I've tested this with both tap-diff and tap-difflet; both seem to assume that if there are expected and actual fields in the diagnostic info, they ought to be diffed and displayed.

Here is a quick and dirty test.js:

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();
});

node test.js tape | tap-difflet

node test.js tape | tap-difflet

node test.js tape | tap-diff

node test.js tape | tap-diff

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 error is going to be used in cases where the test knows it's in a bad state, and thus there isn't really an ‘expected’ value—the expectation is that the test won't run that branch.

For comparison, here's how node-tap fares:

node test.js tap | tap-difflet

node test.js tap | tap-difflet

(node test.js tap | tap-diff produces a honkin' ugly stack trace, so I'll spare you that. 😄)

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