-
Notifications
You must be signed in to change notification settings - Fork 306
Fix regression detecting create-react-app #332
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| jest.unmock('../index') | ||
| import { isDefaultPathToJest } from './index' | ||
|
|
||
| describe('isDefaultPathToJest', () => { | ||
| it('returns true when the value is null', () => { | ||
| expect(isDefaultPathToJest(null)).toBe(true) | ||
| }) | ||
|
|
||
| it('returns true for the legacy default ""', () => { | ||
| expect(isDefaultPathToJest('')).toBe(true) | ||
| }) | ||
|
|
||
| it('returns false otherwise', () => { | ||
| expect(isDefaultPathToJest('')).toBe(false) | ||
| }) | ||
| }) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @seanpoulter after merge this PR, realized you put some tests in the src directory, while the others are in tests directory. Is this intentional? it seems inconsistent... most importantly, the jest.json only look for files within tests directory ("testRegex": "tests/.*\.ts$") today, therefore, the tests within src are not being executed right now. I am ok either way (test or src) as long as they are consistent and executed. please advice...
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, I will move this test into the normal |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ import { platform } from 'os' | |
| import { existsSync, readFileSync } from 'fs' | ||
| import { normalize, join } from 'path' | ||
|
|
||
| import { IPluginSettings } from './IPluginSettings' | ||
| import { IPluginSettings, hasUserSetPathToJest } from './Settings' | ||
|
|
||
| /** | ||
| * Known binary names of `react-scripts` forks | ||
|
|
@@ -11,7 +11,7 @@ const createReactAppBinaryNames = ['react-scripts', 'react-native-scripts', 'rea | |
|
|
||
| /** | ||
| * Tries to read the test command from the scripts section within `package.json` | ||
| * | ||
| * | ||
| * Returns the test command in case of success, | ||
| * `undefined` if there was an exception while reading and parsing `package.json` | ||
| * `null` if there is no test script | ||
|
|
@@ -29,24 +29,24 @@ export function getTestCommand(rootPath: string): string | undefined | null { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| /** | ||
| * Checks if the supplied test command could have been generated by create-react-app | ||
| */ | ||
| export function isCRATestCommand(testCommand: string): boolean { | ||
| export function isCreateReactAppTestCommand(testCommand: string): boolean { | ||
| return testCommand && createReactAppBinaryNames.some(binary => testCommand.indexOf(binary + ' test') === 0) | ||
| } | ||
|
|
||
| /** | ||
| * Checks if the project in `rootPath` was bootstrapped by `create-react-app`. | ||
| */ | ||
| function isBootstrappedWithCRA(rootPath: string): boolean { | ||
| function isBootstrappedWithCreateReactApp(rootPath: string): boolean { | ||
| const testCommand = getTestCommand(rootPath) | ||
| if (testCommand === undefined) { | ||
| // In case parsing `package.json` failed or was unconclusive, | ||
| // fallback to checking for the presence of the binaries in `./node_modules/.bin` | ||
| return createReactAppBinaryNames.some(binary => hasNodeExecutable(rootPath, binary)) | ||
| } | ||
| return isCRATestCommand(testCommand) | ||
| return isCreateReactAppTestCommand(testCommand) | ||
| } | ||
|
|
||
| function hasNodeExecutable(rootPath: string, executable: string): boolean { | ||
|
|
@@ -56,29 +56,32 @@ function hasNodeExecutable(rootPath: string, executable: string): boolean { | |
| } | ||
|
|
||
| /** | ||
| * Handles getting the jest runner, handling the OS and project specific work too | ||
| * Handles getting the jest runner, handling the OS and project specific work too | ||
| * | ||
| * @returns {string} | ||
| */ | ||
| export function pathToJest(pluginSettings: IPluginSettings) { | ||
| if (pluginSettings.pathToJest) { | ||
| if (isBootstrappedWithCRA(pluginSettings.rootPath)) { | ||
| return 'npm test --' | ||
| } | ||
| return normalize(pluginSettings.pathToJest) | ||
| export function pathToJest({ pathToJest, rootPath }: IPluginSettings) { | ||
| if (hasUserSetPathToJest(pathToJest)) { | ||
| return normalize(pathToJest) | ||
| } | ||
|
|
||
| const platform = process.platform | ||
| if (platform === 'win32' && existsSync(join(pluginSettings.rootPath, 'node_modules', '.bin', 'jest.cmd'))) { | ||
| return normalize(join(pluginSettings.rootPath, 'node_modules', '.bin', 'jest.cmd')) | ||
| } else if ( | ||
| (platform === 'linux' || platform === 'darwin') && | ||
| existsSync(join(pluginSettings.rootPath, 'node_modules', '.bin', 'jest')) | ||
| ) { | ||
| return normalize(join(pluginSettings.rootPath, 'node_modules', '.bin', 'jest')) | ||
| if (isBootstrappedWithCreateReactApp(rootPath)) { | ||
| return 'npm test --' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we here also add the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is a known problem but that's not the problem I'm trying to solve. It was intended to be a fast fix to get the extension usable for everyone again. This preserves behaviour from Line 65 on the left, and is same before from before the regression (here).
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I bumped into this problem today while testing another PR. I agree #324 was flawed, but to override If our goal is to provide a "default" setting when BTW, this PR has been sitting for a while, what is the plan? People are adopting custom
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function starts with a I think the best place would be the Jest output window, which also shows the errors, when there's an issue with the setting or our guess about it. The PR is waiting for a review by Orta, but I guess we could skip that since this issue in the meantime got quite old.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
oh that's right, my bad, great!
that might work too... however it currently tries to reset the window (channel) on each jest run, so we will have to output this info at each run, no big deal. BTW, I did seen empty buffer at times, so probably some bugs in our channel reset logic.
I am going to submit a new PR to address jest 23 compatibility issue, if you merged this PR soon, I could pick up your change and test them together (except windows) before we cut the next release... Of course that is if @orta has no objection.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I would like to also include a PR like #297, such that all pathToJest and Windows problems should be fixed. But that should be a matter of minutes as soon as we can merge this PR.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's get them all in, I'll handle the jest ones and get that released in a few days - can we try get those two merged in that timeframe and we'll ship a new release?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. Can we merge this one or is there something speaking against?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stephtr this PR has no @orta If you can merge facebook/jest/6586 and cut a beta jest release, then we can proceed on this end for the PRs depending on the new
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I've requested a release for editor-support - so let's get this PR in then, and #297 can happen whenever |
||
| } | ||
|
|
||
| return 'jest' | ||
| const localJestExecutable = pathToLocalJestExecutable(rootPath) | ||
| if (existsSync(localJestExecutable)) { | ||
| return localJestExecutable | ||
| } | ||
| return `jest${isWindows() ? '.cmd' : ''}` | ||
| } | ||
|
|
||
| function pathToLocalJestExecutable(rootDir) { | ||
| return normalize(join(rootDir, `node_modules/.bin/jest${isWindows() ? '.cmd' : ''}`)) | ||
| } | ||
|
|
||
| function isWindows() { | ||
| return platform() === 'win32' | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this talk about improving maintainability made me remember just how long it took to figure out what "CRA" was from reading the issues. Let's spell it out for folks.