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

Tests are using code from npm instead of code from pull request #5440

Closed
brunolemos opened this issue Oct 15, 2018 · 5 comments
Closed

Tests are using code from npm instead of code from pull request #5440

brunolemos opened this issue Oct 15, 2018 · 5 comments

Comments

@brunolemos
Copy link
Contributor

brunolemos commented Oct 15, 2018

Possibly a regression from 5fecfee.
Blocking #4837.

Is this a bug report?

Yes

Environment

System:
OS: macOS 10.14
CPU: x64 Intel(R) Core(TM) i5-6267U CPU @ 2.90GHz
Binaries:
Node: 8.11.4 - /usr/local/bin/node
Yarn: 1.10.1 - ~/.yarn/bin/yarn
npm: 6.4.1 - /usr/local/bin/npm
Browsers:
Chrome: 69.0.3497.100
Firefox: 59.0.2
Safari: 12.0
npmPackages:
@stiligita/react: 1.0.0-0
@types/react: 16.4.14
@types/react-dom: 16.0.8
react: 16.5.2
react-dom: 16.5.2
react-scripts: 2.0.5 (#4837)
npmGlobalPackages:
create-react-app: Not Found (#4837)

Steps to Reproduce

  • Fork TypeScript support using Babel 7 #4837
  • Run CI=false npx jest --config ./test/jest.config.js --testMatch '<rootDir>/**/typescript/*.test.js' (pass)
  • Run CI=true npx jest --config ./test/jest.config.js --testMatch '<rootDir>/**/typescript/*.test.js' (fail)

Expected Behavior

Tests should pass because they are correct.
CRA should use the current code of each pull request, not a version from npm.

Actual Behavior

TypeScript tests fails because it tries to use the cra version from npm, which doesn't support typescript.

Reproducible Demo

#4837

Possibly related code

this.isLocal = !(process.env.CI && process.env.CI !== 'false');

const shouldInstallScripts = !this.isLocal;
if (shouldInstallScripts) {
packageJson.dependencies = Object.assign({}, packageJson.dependencies, {
'react-scripts': 'latest',
});
}

@brunolemos brunolemos changed the title Regression: Tests are using code from npm instead of code from pull request Tests are using code from npm instead of code from pull request Oct 15, 2018
@Timer
Copy link
Contributor

Timer commented Oct 15, 2018

The tests are supposed to be using code from "npm" because the PR is published before the test are ran:

# Publish the monorepo
git clean -df
./tasks/publish.sh --yes --force-publish=* --skip-git --cd-version=prerelease --exact --npm-tag=latest
# ******************************************************************************
# Now that we have published them, run all tests as if they were released.
# ******************************************************************************
# Run all tests
cd test/
CI=true ../node_modules/.bin/jest -w 2

I suspect our registry override might be wiped out, or similar.

@Timer
Copy link
Contributor

Timer commented Oct 15, 2018

https://travis-ci.org/facebook/create-react-app/jobs/441577719#L606-L709 this looks suspicious. Nothing but yarn.lock should be removed.

@brunolemos
Copy link
Contributor Author

brunolemos commented Oct 15, 2018

I made the wrong diagnostic about the problem because locally I ran jest directly so it didn't make this publish to the local npm.

I added a console.log to see the real test error on travis: https://travis-ci.org/facebook/create-react-app/jobs/441692012#L787-L797 (btw I wish it showed this by default):

 FAIL  fixtures/typescript/index.test.js (25.768s)
  ● Console
    console.error fixtures/typescript/index.test.js:7
      { Error: Command failed: yarnpkg test --env node --ci
      FAIL src/App.test.ts
        ● Test suite failed to run
      
          Cannot find module './App' from 'App.test.ts'
      
          > 1 | import App from './App';

It loaded App.test.ts so it's using the correct code from the PR.
But App.test.ts is not being able to import ./App which is weird.

@brunolemos
Copy link
Contributor Author

brunolemos commented Oct 15, 2018

Adding .disable-pnp fixed it 🤔🎉

@Timer
Copy link
Contributor

Timer commented Oct 15, 2018

Hmm, I'd like to figure out why this doesn't work with PnP (it should).

@lock lock bot locked and limited conversation to collaborators Jan 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants