Fix "container is falsey" error when using code coverage #156
Conversation
|
Node 6 does not like object spreads. |
|
Ya, I had some misconceptions about how the Istanbul plugin was getting things done. |
| // babel-plugin-istanbul won't run on <= Node 6 | ||
| const majorVersion = parseInt(process.version.match(/^v(\d+)\./)[1], 10); | ||
| const runOrSkip = majorVersion > 6 ? it : it.skip; |
__tests__/index-test.js
Outdated
| expect(() => { | ||
| babel7.transformSync(source, { | ||
| filename: 'istanbul-should-cover.js', | ||
| plugins: [require('babel-plugin-istanbul'), Plugin], | ||
| }); | ||
| }).toThrow(/Container is falsy/i); |
There was a problem hiding this comment.
Can you tweak this to something like:
| expect(() => { | |
| babel7.transformSync(source, { | |
| filename: 'istanbul-should-cover.js', | |
| plugins: [require('babel-plugin-istanbul'), Plugin], | |
| }); | |
| }).toThrow(/Container is falsy/i); | |
| expect(() => { | |
| babel7.transformSync(source, { | |
| filename: 'istanbul-should-cover.js', | |
| plugins: [require('babel-plugin-istanbul'), Plugin], | |
| }); | |
| }).toMatchInlineSnapshot(); |
Basically, we don't want it to throw...
There was a problem hiding this comment.
To make sure I'm tracking, the idea is to provide feedback that's a bit more useful than "it did/not throw"?
There was a problem hiding this comment.
Right, exactly. Basically expect().toThrow() is not what we want here, we want to say it does not throw (IOW, this PR should be failing not passing). Using .toMatchInlineSnapshot just lets us confirm that it emits something that we can visually verify "looks correct".
|
Poking some more, it looks like this is the root cause. |
|
Standing on bigger shoulders than my own, this seems like a commonly accepted solution - istanbuljs/babel-plugin-istanbul#192 Don't know if it's the most efficient place to put it though. |
…ber-modules-api-polyfill � Conflicts: � package.json � yarn.lock
src/index.js
Outdated
| @@ -165,6 +165,8 @@ module.exports = function (babel) { | |||
| ]) | |||
| ); | |||
| } else { | |||
| path.scope.crawl(); | |||
There was a problem hiding this comment.
I worry that doing this for every import declaration replacement is going to be quite costly. Now that you fully understand what is wrong, I wonder if we can avoid the re-crawl by detecting the specific failure scenario and handling it. 🤔
Thoughts @mdeanjones?
There was a problem hiding this comment.
Makes sense. I opted for a "try, and try again" approach. It gets the job done, but does feel a bit... hacky.
…ber-modules-api-polyfill � Conflicts: � __tests__/index-test.js � package.json � src/index.js � yarn.lock
rwjblue
changed the title
|
Thank you @mdeanjones! |
#112