Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow instrumentation of transformed files with weird file extensions #9589

Merged
merged 2 commits into from
Feb 19, 2020

Conversation

andersk
Copy link
Contributor

@andersk andersk commented Feb 18, 2020

Summary

The default configuration of babel-plugin-istanbul only allows instrumentation of files with extensions .js, .cjs, .mjs, .ts, .tsx, .jsx (https://github.com/istanbuljs/schema/blob/v0.1.2/index.js#L71). However, we know that we’re running it on code that we just transformed to JavaScript, so the source language no longer matters, and we should disable this check to get complete instrumentation.

Test plan

With the example project at https://github.com/andersk/jest-handlebars-test, before:

$ jest --coverage --no-cache
 PASS  __tests__/greet.js
  ✓ am (3ms)

----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |       0 |        0 |       0 |       0 |                   
----------|---------|----------|---------|---------|-------------------
Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        0.443s
Ran all test suites.

and after:

$ jest --coverage --no-cache
 PASS  __tests__/greet.js
  ✓ am (4ms)

-----------|---------|----------|---------|---------|-------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-----------|---------|----------|---------|---------|-------------------
All files  |      75 |    66.67 |   66.67 |   66.67 |                   
 greet.hbs |      75 |    66.67 |   66.67 |   66.67 | 4                 
-----------|---------|----------|---------|---------|-------------------
Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        0.551s
Ran all test suites.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This makes sense to me. Could you add an integration test as well? We have some for coverage already

@andersk
Copy link
Contributor Author

andersk commented Feb 19, 2020

Okay, I’ve added my example project as an integration test.

@andersk andersk requested a review from SimenB February 19, 2020 07:37
@drew-gross

This comment has been minimized.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thank you! Just running prettier/eslint on the one file and we should be good to go 🙂

@andersk andersk force-pushed the instrument-extension branch 2 times, most recently from 917b8f0 to b6b8308 Compare February 19, 2020 08:18
@andersk
Copy link
Contributor Author

andersk commented Feb 19, 2020

Just to make sure I did the right thing here: I added an exemption to checkCopyrightHeaders for e2e/coverage-handlebars. (I don’t care so much, but since the CLA isn’t a copyright transfer, I didn’t want to claim that my code is © Facebook.)

I left the copyright header on e2e/__tests__/coverageHandlebars.test.ts, which is copied from coverageRemapping.test.ts and coverageTransformInstrumented.test.ts.

@SimenB
Copy link
Member

SimenB commented Feb 19, 2020

I honestly have no idea what the header is for, but just adding the header seems easiest. The blacklist is for stuff like vendored modules and config files, I think, but this is code (albeit tests)

@codecov-io
Copy link

Codecov Report

Merging #9589 into master will increase coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9589      +/-   ##
==========================================
+ Coverage   65.06%   65.07%   +0.01%     
==========================================
  Files         286      287       +1     
  Lines       12137    12141       +4     
  Branches     3007     3008       +1     
==========================================
+ Hits         7897     7901       +4     
  Misses       3605     3605              
  Partials      635      635
Impacted Files Coverage Δ
packages/jest-transform/src/ScriptTransformer.ts 69.64% <ø> (ø) ⬆️
e2e/coverage-handlebars/greet.hbs 50% <50%> (ø)
packages/expect/src/utils.ts 96.22% <0%> (+1.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ed46e7...b6b8308. Read the comment docs.

@andersk andersk force-pushed the instrument-extension branch from b6b8308 to 70fd2de Compare February 19, 2020 08:36
@andersk
Copy link
Contributor Author

andersk commented Feb 19, 2020

Eh, I guess I’m willing to claim to be an “affiliate” for this purpose. Updated.

@drew-gross

This comment has been minimized.

@andersk andersk force-pushed the instrument-extension branch 2 times, most recently from d8714fe to 80077a9 Compare February 19, 2020 09:02
@codecov-io
Copy link

Codecov Report

Merging #9589 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9589      +/-   ##
==========================================
- Coverage   65.06%   65.06%   -0.01%     
==========================================
  Files         286      287       +1     
  Lines       12137    12141       +4     
  Branches     3007     3010       +3     
==========================================
+ Hits         7897     7899       +2     
- Misses       3605     3606       +1     
- Partials      635      636       +1
Impacted Files Coverage Δ
packages/jest-transform/src/ScriptTransformer.ts 69.64% <ø> (ø) ⬆️
e2e/coverage-handlebars/greet.hbs 50% <50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ed46e7...80077a9. Read the comment docs.

The default configuration of babel-plugin-istanbul only allows
instrumentation of files with extensions .js, .cjs, .mjs, .ts, .tsx,
.jsx (https://github.com/istanbuljs/schema/blob/v0.1.2/index.js#L71).
However, we know that we’re running it on code that we just
transformed to JavaScript, so the source language no longer matters,
and we should disable this check to get complete instrumentation.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@andersk andersk force-pushed the instrument-extension branch from 80077a9 to 443734c Compare February 19, 2020 09:27
@drew-gross

This comment has been minimized.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants