Respect frame on plugin thrown errors when pos is also given - #2309
Conversation
guybedford
left a comment
There was a problem hiding this comment.
A test would be a huge help, because these behaviours are quite loosely defined otherwise and at risk of regression.
How about a simple API-based test in test/hooks?
|
|
||
| if (object.frame === undefined) { | ||
| const { line, column } = object.loc; | ||
| object.frame = object.frame || getCodeFrame(source, line, column); |
There was a problem hiding this comment.
This can just be object.frame = getCodeFrame( then?
ad66d28 to
a27a65f
Compare
Obviously, this was last second refactor 😅
Gonna prepare one in following days. Newborn in the house 😉 |
a27a65f to
80920cc
Compare
|
@guybedford added a CLI test |
lukastaegert
left a comment
There was a problem hiding this comment.
Thanks & looks good, my suggestions are only towards improving the test a little.
| @@ -0,0 +1 @@ | |||
| console.log("everyday I'm throwing"); | |||
| @@ -43,6 +43,11 @@ describe('cli', () => { | |||
|
|
|||
| if ('stderr' in config) { | |||
| assert.equal(deindent(config.stderr), stderr.trim()); | |||
There was a problem hiding this comment.
Not actually something you did but your test made me notice that the two arguments should be switched as otherwise the error messages will have "actual" and "expected" confused.
There was a problem hiding this comment.
Also, there is another issue I am experiencing here, which did not matter before as the only other test using stderr was only checking if it is empty: Depending on the environment where I run the test, i.e. from terminal or from my IDE, it can happen that ANSI escape sequences are injected into the output. This will unfortunately turn the test red 😢
My suggestion, which would also make config.stderrOnly superfluous, would be to make config.stderr work like config.error i.e. instead of a string we expect a function here that receives stderr and returns if the test should continue afterwards. Then you could e.g. do RegExp matching on the output and ignore possibly existing ANSI sequences. The only other test cli/samples/silent that uses stderr should be easy to convert.
There was a problem hiding this comment.
BTW for reference, this is the output I get with escape sequences (the non-printable characters are the escape characters):
�[36m�[39m
�[36m�[1mmain.js�[22m → �[1mstdout�[22m...�[39m
�[1m�[31m[!] �[1mError: My error.�[1m�[39m�[22m
main.js (1:5)
�[2mcustom code frame�[22m
| line: 1, | ||
| column: 5, | ||
| }; | ||
| err.frame = 'custom code frame'; |
There was a problem hiding this comment.
It puzzled me a little why your test was green even without your changes to the code and it seems this test does not even trigger your code changes as no code position is provided as the second argument. This does not mean that I would change this test—it definitely tests something important—but that there should be a second test where this.error is called with a second argument, either a {line, column} object or a plain number (or just call it twice :) you might also just put all three invocations into one test).
And congratulations! |
80920cc to
339ecc9
Compare
|
@lukastaegert I've made suggested amends. Would also appreciate you taking a look at this
Thanks! :) |
Would love to add a test for this - hadn't yet chance to search for similar things being tested though.