improve yaml formating of diagnostic information #171
Conversation
|
I can add more tests and examples of things that are actually broken tomorrow. I wanted to get this up before heading home for the night. the main issue is any time there is an indicator character in one of the yaml fields it will throw off most (all?) yaml parsers. |
|
I have added a few more tests and simplified the logic a bit. If either the actual or the expected value in the diagnostic yaml block contain |
|
@Raynos I think this PR is in a state where its ready for feedback, have not been able to think of any other ways to minimize the changes to the existing output format |
| @@ -4,6 +4,7 @@ var through = require('through'); | |||
| var resumer = require('resumer'); | |||
| var inspect = require('object-inspect'); | |||
| var hasOwn = Object.prototype.hasOwnProperty; | |||
| var invalidYaml = RegExp.prototype.test.bind(/\:|\-|\?/); | |||
There was a problem hiding this comment.
Using .bind breaks ES3 compatibility. Can you use the function-bind module here instead?
Alternatively, don't use bind, capture RegExp#test in a closure, and make a wrapper function that .calls it - but that's less robust against Function#call being broken.
There was a problem hiding this comment.
ah, good point. I'll fix that
|
rereading my previous comment, it kinda comes across as impatient. I was trying to speak to the fact that in its previous state my PR was incomplete and I felt it need more tests and research into how yaml can be formatted. |
|
@hayes as I understand the aim here is to emit valid yaml for parsers. Can you add a yaml parser to dev deps and use it in the tests to assert valid yaml |
|
@Raynos I suppose I should do that. tap-parser internally uses a yaml parser to parse the diagnostic output, which avoids me manually grabbing that part of the output, but I'd be happy to switch it out if you think it would be better to use a yaml parser directly |
|
@ljharb all of these defend in depth to insanity suggestions don't help the code base. If you want to break ES5 then don't use tape. |
|
@Raynos If I want to test my module in an environment where someone else broke ES5 i have to simulate that by breaking it. I'm not sure why it doesn't help the codebase to make it less likely to break, especially when it's pretty easy to handle. If you're concerned about readability, we should be using deps that already handle it for us - like |
|
@Raynos I've added tests for parsing yaml directly for both stack traces and expected/actual values |
|
My opinion is that if you want to test your module write a node script that does Like if your doing some seriously niche stuff; write the tests like they are written in node core itself. A test is a program that exits 0 or 1. Also adding more modules is even worse. We don't need more modules. |
| @@ -60,7 +62,7 @@ Results.prototype.createStream = function (opts) { | |||
| output.queue('TAP version 13\n'); | |||
| self._stream.pipe(output); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
can you revert whitespace changes. The local style is "special"
|
The whole point of using |
|
This changes looks good to me; lets get the whitespace changes reverted though. I'll get @substack opinion on this because I know he has strong opinions around yaml. |
hayes
force-pushed
the
fix-stack-yaml
branch
3 times, most recently
from
6625e9a to
e9749b0
Compare
|
@Raynos thanks, reverted whitespace changes |
|
@hayes thanks; looking good. |
|
This patch looks good. |
improve yaml formating of diagnostic information
|
Published v4.1.0 |
|
awesome, thank you! |
I am not sure this is the best way to do this, since I am not too familiar with yaml, but I wanted to at least get an opinion on this issue. I am trying to find a way that has the smallest impact on existing behavior, but I believe the diagnostic part of tapes output should contain valid yaml
Currently tape produces invalid yaml for many errors and assertions. This makes parsing the output very difficult.
This PR changes tapes output so that the
expectedandactualvalues in diagnostic yaml will be wrapped in double quotes (and double quotes will be prepended by a\) IFF the value contains a yaml indicator character (?,:or-). This affects the output for most object comparisons.It also updates the format of the
stackproperty. Currently stack traces are emitted as a folded multiline string that is collapsed when parsed as yaml, this PR changes it to be emitted as a literal multiline string, so when parsed as yaml, it newlines will be preserved.