feat(jest-transformer): generate jest tests sourcemaps #706
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark resultsBase commit: lwc-engine-benchmark
|
|
Thank you for this! 😄 🎆 |
pmdartus
requested changes
Oct 4, 2018
|
|
||
| const generated = babelCore.transform(code, BABEL_CONFIG); | ||
| if (isJsFile) { | ||
| config.inputSourceMap = map; |
There was a problem hiding this comment.
Why not always add the sourcemap as input?
| @@ -30,15 +31,25 @@ const BABEL_CONFIG = { | |||
|
|
|||
| module.exports = { | |||
| process(src, filePath) { | |||
| const isJsFile = path.extname(filePath) === '.js'; | |||
| // generate sourcemaps only for js files, otherwise the coverage is affected with template code. | |||
| const config = Object.assign({}, BABEL_CONFIG, { sourceMaps: isJsFile ? 'inline' : false }); | |||
There was a problem hiding this comment.
Move the config declaration after line 46 to avoid mutating it's value.
| config.inputSourceMap = map; | ||
| } | ||
|
|
||
| const generated = babelCore.transform(code, config); |
There was a problem hiding this comment.
It appears that you can return babel-core result directly: https://github.com/facebook/jest/blob/master/packages/babel-jest/src/index.js#L145-L147
There was a problem hiding this comment.
ill verify but makes sense.
| @@ -30,15 +31,25 @@ const BABEL_CONFIG = { | |||
|
|
|||
| module.exports = { | |||
| process(src, filePath) { | |||
| const isJsFile = path.extname(filePath) === '.js'; | |||
| // generate sourcemaps only for js files, otherwise the coverage is affected with template code. | |||
| const config = Object.assign({}, BABEL_CONFIG, { sourceMaps: isJsFile ? 'inline' : false }); | |||
There was a problem hiding this comment.
Why not always returning the sourcemap?
There was a problem hiding this comment.
cause that will include the template javascript generated code on the coverage. Since i dont think the template code should be included in the coverage, i disabled the map generation since it will add extra calculations for the template.
your thoughts @trevor-bliss?
There was a problem hiding this comment.
@trevor-bliss after checking with @pmdartus, we are fine adding the sourcemaps for the .html and the .css as long as we only add coverage for .js files
Just one thing @pmdartus , the sourcemap file name for html and css should be different from the original, for example with .compiled appended, otherwise wont be debug-able in the IDE (cause it will show the original file content).
There was a problem hiding this comment.
Agree we should not include coverage for the html and css files if we include them.
It did feel weird to me from a test debugging standpoint to include the html though. In the example Jose demoed to me, if you set a breakpoint in a js function of your component, when you return from that function you are back in compiled html land. I can see this being useful to the lwc core team or someone debugging a template compiler issue, but the the internal rendering APIs feel cryptic and unnecessary to the average unit test debugger. I could be underestimating our customers willingness to understand and debug the internals though.
There was a problem hiding this comment.
actually currently we need to add:
coveragePathIgnorePatterns: [ '.html$', '.css$' ],to the jest config in order to have a more accurate coverage number (if we dont want to include the generated code for the template/css in the coverage)
based on that, the remaining question is: while debugging, do we want to see the actual code that is being executed (the compiled code) or the template with no idea of what is being happening? from my dev standpoint mmm yeah what is executed.
There was a problem hiding this comment.
Yeah after trying this out myself I think we should include the html and css and just exclude it from coverage.
| moduleNamespace: 'x' | ||
| moduleNamespace: 'x', | ||
| outputConfig: { | ||
| sourcemap: isJsFile |
There was a problem hiding this comment.
Jest seems to support both
https://github.com/facebook/jest/blob/master/packages/babel-jest/src/index.js#L85
There was a problem hiding this comment.
this is just to tell the lwc-compiler to generate sourcemaps for this transformation (a map object). the both option is for the babel transform, if we want both (inline and a map object) which i dont see the need, but neither the harm.
There was a problem hiding this comment.
tracing back the both option in the babel-jest:
inline is still supported but has some performance issues: jestjs/jest#5177 (comment)
they support returning a map from the processor, and if that map is returned, they dont look to the inline one, that is why inline -> both in babel-jest.
Setting it to both on the babel config. nice catch @pmdartus!!
Generate component sourcemaps in jest tests.
jodarove
force-pushed
the
jodarove/sourcemap-jest-transformer
branch
from
00e3962
to
5e044af
Compare
to exclude .html and .css from being included in the jest coverage report.
jodarove
force-pushed
the
jodarove/sourcemap-jest-transformer
branch
from
5e044af
to
ea6a734
Compare
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
trevor-bliss
approved these changes
Oct 5, 2018
pmdartus
approved these changes
Oct 5, 2018
|
|
||
| const generated = babelCore.transform(code, BABEL_CONFIG); | ||
| // if is not .js, we add the .compiled extension in the sourcemap | ||
| const filename = path.extname(filePath) === '.js' ? filePath : filePath + '.compiled'; |
|
|
||
| return generated.code; | ||
| return babelCore.transform(code, { ...BABEL_CONFIG, ...{ filename }, ...config }); |
There was a problem hiding this comment.
You can further simply this code:
return babelCore.transform(code, {
...BABEL_CONFIG,
...config,
filename,
});
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Generate component sourcemaps in jest tests, which improve the coverage, and make the tests debuggable.
Does this PR introduce a breaking change?