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

Use cross-spawn for running Yarn in integration tests #5550

Merged
merged 5 commits into from
Feb 13, 2018

Conversation

thymikee
Copy link
Collaborator

Summary

Use cross-spawn for running Yarn in integration tests, which should unblock running some tests on Windows.

Test plan

Observe AppVeyor

const {cleanup, run} = require('../utils');
const runJest = require('../runJest');

const dir = path.resolve(__dirname, '../coverage-remapping');
const coverageDir = path.join(dir, 'coverage');

SkipOnWindows.suite();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this is the only test that spawns different results on windows - it adds covered-test.ts next to covered.ts. Ideas what can cause that? cc @SimenB @jwbay @Aftabnack

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/facebook/jest/blob/2df9e4cc4b93abc71659c216c07d1a6c78909f0b/packages/jest-runner/src/run_test.js#L126-L127

  • Here it gets the coverage and soucemap info from the from the runner, coverage will be there only for files being used in the tests.
  • But soucemaps, along with containing maps for tested code, it also contains mapping for test file itself.

https://github.com/facebook/jest/blob/2df9e4cc4b93abc71659c216c07d1a6c78909f0b/packages/jest-cli/src/reporters/coverage_reporter.js#L55-L77

  • Here it iterates over sourcemaps and tries to add it to the coverage map.

My guess, this addition works for test file itself, which it doesn't in any other testcase. Do we have another testcase which is written in ts, same behaviour should be seen

@codecov-io
Copy link

codecov-io commented Feb 13, 2018

Codecov Report

Merging #5550 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5550   +/-   ##
=======================================
  Coverage   61.71%   61.71%           
=======================================
  Files         213      213           
  Lines        7149     7149           
  Branches        3        4    +1     
=======================================
  Hits         4412     4412           
  Misses       2736     2736           
  Partials        1        1

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 2df9e4c...111e1ba. Read the comment docs.

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

🔥

@cpojer cpojer merged commit b69ac08 into jestjs:master Feb 13, 2018
@thymikee thymikee deleted the chore/appveyor-yarn branch February 13, 2018 18:13
jessecarfb pushed a commit to jessecarfb/jest that referenced this pull request Feb 14, 2018
* Use cross-spawn for running Yarn in integration tests

* Unlock more windows tests

* Moar windows

* Use node api instead of spawning commands to link babel-jest

* Skip windows for coverage_remapping.test.js
@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 12, 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