add output for failing tests #837
Conversation
|
@cgcgbcbc - see avajs/eslint-plugin-ava#110 (comment) for the desired mini reporter format. Don't worry about figuring out how to gather the issue links. We will figure that out in #836 . You can ignore the issue links part entirely for this PR if you want. Or you can check for it conditionally. if (test.issueLinks) {
test.issueLinks.forEach(...)
} |
I prefer to ignore this part first 😄 |
lib/reporters/mini.js
Outdated
| } else { | ||
| this.passCount++; | ||
| if (test.failing) { | ||
| this.failCount++; |
There was a problem hiding this comment.
Did you mean failingCount?
There was a problem hiding this comment.
Maybe rename "failing" to knownFailure / knownFailureCount.
|
@novemberborn updated. As for the verbose reporter, it seems that it uses the count in runStatus (which in mini reporter it keeps track by itself). https://github.com/sindresorhus/ava/blob/master/lib/reporters/verbose.js#L74-L81 So when are the count added to runStatus? As perhaps I need to modify that for counting known failure. |
|
seems that I should just modify run-status? But that would influence a lot others? |
It'll be fine. You're adding a new property anyhow. Changes looking good so far @cgcgbcbc! |
|
@cgcgbcbc - We want to land this before cutting a new release. Any chance you can finish this up in the next day or two? |
Ok, I'll try my best to finish this tonight(GMT +8) |
| } | ||
|
|
||
| if (test.failing && !test.error) { | ||
| this.knownFailures.push(test); |
There was a problem hiding this comment.
where should I write tests for this line? Seems that there is no test/run-status.js
There was a problem hiding this comment.
@cgcgbcbc, @nfcampos is running into that as well: #844 (comment).
Maybe leave that for now, focus on the reporter tests (which use stubbed data anyway).
cgcgbcbc
changed the title
|
@novemberborn I've covered the uncovered lines in reporters, but I suppose there must be more tests needed, can you give me some advice? |
lib/reporters/mini.js
Outdated
| i++; | ||
|
|
||
| var title = test.title; | ||
| // var description = JSON.stringify(test); |
There was a problem hiding this comment.
I think you forgot to delete this
|
It's strange that ci build failed for my last commit which only remove a comment.. |
test/reporters/mini.js
Outdated
| }; | ||
| var actualOutput = reporter.finish(runStatus); | ||
| var expectedOutput = [ | ||
| '\n ' + chalk.green('1 passed') + time, |
There was a problem hiding this comment.
Just remove time. It only prints in watch mode now
|
See https://gist.github.com/novemberborn/8d0fabc0eefe26e055c8239ff8e26a29 for my test script. I tried it with the mini reporter and |
|
@novemberborn now the output are like this |
|
I like the red check for the expected failure in verbose mode. Nice touch! |
So after flipping the test result, reason need to be deleted if the flipped result is passed.
|
|
||
| var expectedOutput = ' ' + chalk.red(figures.tick) + ' ' + chalk.red('known failure'); | ||
|
|
||
| t.is(actualOutput, expectedOutput); |
There was a problem hiding this comment.
Why this assertion failed..
There was a problem hiding this comment.
Change the assertion to
t.is(JSON.stringify(actual), JSON.stringify(expected)
There was a problem hiding this comment.
I think colors are the issue here.
There was a problem hiding this comment.
Yes. My suggestion above will make the color chars visible
There was a problem hiding this comment.
Did not see your comment when I posted mine ;)
|
|
|
LGTM |
|
Really great work on this @cgcgbcbc 👌 Thank you! |
|
|
5 participants
see #673