Fix regression detecting create-react-app #332

seanpoulter · 2018-05-22T03:59:37Z

As discussed in Issue #313 and the comments on PR #324, we've introduced a regression that broke support for create-react-app out of the box.

Changes proposed in this PR:

This changes when we check if the workspace was bootstrapped with create-react-app. We've recently changed the default value for pathToJest to null so the following check would only be performed if the user has changed their settings. This caused new bootstrapped projects to run with jest instead of indirectly calling react-scripts test via npm test --.

The following caused the regression:
```
   if (pluginSettings.pathToJest) {	
    if (isBootstrappedWithCRA(pluginSettings.rootPath)) {	
```

Testing plan:

Tested on OSX without issue
Added tests for pathToJest to capture recent changes including defaulting to global Jest

coveralls · 2018-05-22T04:01:56Z

Pull Request Test Coverage Report for Build 403

14 of 14 (100.0%) changed or added relevant lines in 4 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+4.1%) to 79.763%

Files with Coverage Reduction	New Missed Lines	%
src/helpers.ts	1	91.46%

Totals
Change from base Build 394:	4.1%
Covered Lines:	669
Relevant Lines:	814

💛 - Coveralls

stephtr · 2018-05-22T06:51:40Z

src/helpers.ts

@@ -61,24 +61,31 @@ function hasNodeExecutable(rootPath: string, executable: string): boolean {
 * @returns {string}
 */
 export function pathToJest(pluginSettings: IPluginSettings) {
+  if (isDefaultSettingForPathToJest(pluginSettings.pathToJest) && isBootstrappedWithCRA(pluginSettings.rootPath)) {


If you really would like to introduce a function for essentially checking if the string is empty, four lines later you also should use that function.

I would instead move the if(pluginSettings.pathToJest) up to the top, so that the remaining part (including the CRA check) only runs if we already know for sure that we are on the default setting (like I proposed it within #297#L64).

If you really would like to introduce a function for essentially checking if the string is empty

It might seem like a trivial thing to make a function for but the function adds value by:

clearly communicating what we're doing

isolating the default value so it's easier to change

providing a seam to test if we're checking the default value properly

A null check often doesn't provide the context that someone needs to understand where the value came from or what the value means. This can be fixed using (something like a Null Object or Special Case), a comment, or a well named variable or function.

I'd like to help build a community that's engaged and empowered to contribute to this extension. It starts with little things like the trivial function or advocating for code that's easier to maintain in the long run. It may be more verbose but it's a small overhead now that should help us all maintain it in the long run.

four lines later you also should use that function

Do you mean to check if the user has changed the value in if (pluginSettings.pathToJest)? That's reordered and changed it to if (isNotDefaultPathToJest(pathToJest)).

move the if(pluginSettings.pathToJest) up to the top

Great idea 👍.

stephtr · 2018-05-22T06:54:35Z

src/helpers.ts

-  const defaultPath = normalize(join(pathToNodeModules, 'jest/package.json'))
-  const cliPath = normalize(join(pathToNodeModules, 'jest-cli/package.json'))
-  const craPath = normalize(join(pathToNodeModules, 'react-scripts/node_modules/jest/package.json'))
+  const defaultPath = normalize(join(pathToNodeModules, 'jest', 'package.json'))


normalize automatically replaces slashes on Windows, so that change wouldn't be necessary, but there's also nothing which speaks against it.

Thanks. I didn't know that. I've reverted those changes.

stephtr · 2018-05-22T06:58:44Z

src/helpers.ts

+}
+
+function pathToLocalJestExecutable(rootDir) {
+  return normalize(join(rootDir, 'node_modules', '.bin', `jest${isWindows() ? '.cmd' : ''}`))


Do we really want to keep adding the .cmd extension on Windows, wasn't the intention of #98 to remove it? Or do you then want to remove that file extension with #297?

I don't have the spare time to re-review #98. Could you please summarize your concerns?

As for the suffix, I think @ThomasRooney put this in as a hotfix for the current behaviour. We don't have the latest version of Jest or that shell option in production just yet. I'm just trying to do folks a favour and put out the fire. 🚒

My approach was to update the pathToJest function and add the missing useShell flag all in one go (which would eliminate the need for the .cmd extension), but that's not necessary, separating it into two PRs also has its advantages.

stephtr · 2018-05-22T07:06:30Z

tests/helpers.test.ts

+      mockNormalize.mockReturnValueOnce(expected)
+
+      const settings: any = {
+        pathToJest: 'jest',


It's just a small thing, but maybe we should test for a different value since jest is the default outcome of the pathToJest function.

Good call. I've switched it to a {}.

stephtr · 2018-05-22T07:09:09Z

src/helpers.ts

+}
+
+function isDefaultSettingForPathToJest(str) {
+  return str === null


It practically doesn't make any large difference, but wouldn't it be nicer if we change the default value of jest.pathToJest within package.json to "" instead of null, like jest.pathToConfig and jest.rootPath?

I'm going to call you out on a pattern in your comments @stephtr. I find it frustrating when you structure your feedback as "wouldn't it be better if ..." or "wouldn't it be nicer if ..." and don't explain why you favour your suggestion. If you provided an explanation I'd be willing to discuss the options. Without one it comes across to me like you know better, and I should too.

For these default values feel free to change them to whatever you want in another PR. It doesn't make any difference to me. I'd lean towards not changing the default JavaScript behaviour and have values default to undefined when they're not provided, or maybe have a look if VS Code suggests any other conventions with their configuration API.

Sorry and thank you for the feedback.
In that case my argument was that jest.pathToConfig, jest.rootPath and jest.pathToJest all are strings, but the first two have default value "", while jest.pathToJest has null as a default value. I will file a PR for changing all of them to null (which seems to be preferred amongst VS Code extensions).

Independent from that, I would prefer return !!str; (or similar) instead of a strict, explicit null check, since for example an empty string (which previously was the default value) is definitely not a valid jest command and better should be attended as if it was the default setting.

seanpoulter · 2018-05-23T02:12:57Z

src/DebugConfigurationProvider.ts

@@ -63,7 +63,7 @@ export class DebugConfigurationProvider implements vscode.DebugConfigurationProv
    }

    const testCommand = folder && getTestCommand(folder.uri.fsPath)
-    if (isCRATestCommand(testCommand)) {
+    if (isCreateReactAppTestCommand(testCommand)) {


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.

seanpoulter · 2018-05-23T02:15:54Z

src/Settings/index.ts

+
+export function isNotDefaultPathToJest(str) {
+  return !isDefaultPathToJest(str)
+}


I've extracted the trivial function and it's complement to join the settings interface.

seanpoulter · 2018-05-23T02:16:35Z

src/helpers.ts

-    return normalize(pluginSettings.pathToJest)
+export function pathToJest({ pathToJest, rootPath }: IPluginSettings) {
+  if (isNotDefaultPathToJest(pathToJest)) {
+    return normalize(pathToJest)
  }


Moved as suggested @stephtr. ☝️ Thanks.

Your argument is good, I just believe that if (pathToJest) ("if pathToJest is set") would be (even for outsiders) more easily readable and understandable than if (isNotDefaultPathToJest(pathToJest)).

I'll change it to if (hasUserSetPathToJest(pathToJest)) and call it a day.

seanpoulter · 2018-05-25T21:43:01Z

Does that address your concerns @stephtr?

stephtr · 2018-05-26T14:42:28Z

src/helpers.ts

-  ) {
-    return normalize(join(pluginSettings.rootPath, 'node_modules', '.bin', 'jest'))
+  if (isBootstrappedWithCreateReactApp(rootPath)) {
+    return 'npm test --'


Shouldn't we here also add the .cmd file extension on Windows? (at least until #297 hopefully solves all extension related issues)

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).

I bumped into this problem today while testing another PR. I agree #324 was flawed, but to override jest.pathToJest without checking if it has already been customized by users will be problematic too, for example, consider for yarn users whose jest.pathToJest was set to yarn test, now it will be silently override...

If our goal is to provide a "default" setting when jest.pathToJest is null, then let's do just that, no more and no less. In general, we should never override users' custom config programmatically (we can suggest, but they should be the one to carry out the change, simply because we could be wrong) and definitely not silently. Actually, giving jest.pathToJest is so critical, if we did end up using the default value, I think we should make this information available some where (maybe a new channel: vscode-jest?), we could also use the new channel to communicate the plugin specific info such as workspace info, settings, recommedations etc.

BTW, this PR has been sitting for a while, what is the plan? People are adopting custom jest.pathToConfig that made the regression issue less urgent, but nevertheless we could take this opportunity to improve quality and reduce confusion...

The function starts with a if (isNotDefaultPathToJest(pathToJest)) return normalize(pathToJest), so there isn't any possibility to override the user setting anymore.

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.

so there isn't any possibility to override the user setting anymore.

oh that's right, my bad, great!

I think the best place would be the Jest output window

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.

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.

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.

I could pick up your change and test them together (except windows) before we cut the next release…

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.

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?

Sure. Can we merge this one or is there something speaking against?

@stephtr this PR has no jest-editor-supports dependency, if you and @seanpoulter all have tested it and are satisfied with it, then it's probably safe to merge, IMHO.

@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 jest-editor-supports.

OK, I've requested a release for editor-support - so let's get this PR in then, and #297 can happen whenever

stephtr · 2018-05-26T14:49:35Z

tests/helpers.test.ts

+    })
+
+    it('returns the normalized "pathToJest" setting when set by the user', () => {
+      const expected = {}


I find it more complicate to use an object and mock normalize than using a string like 'customjest', but that's just my personal opinion and nothing which has to be changed.

It's not great using the wrong type, but since it's an Object that's asserted with expect().toBe() you can be confident the value hasn't changed.

stephtr · 2018-05-26T15:05:20Z

Sorry for the delay, unfortunately I had an even busier week than usually.

I have only one concern, the explicit null check in isDefaultPathToJest.
Not that long ago the default value for jest.pathToJest was "", it could therefore be possible that some users have "" in their local VS Code configurations. The explicit null check would break the plugin for them, since it would then try to execute an empty string.
Apart from that it looks good to me.

stephtr · 2018-06-10T19:39:55Z

@seanpoulter Do you have an ETA or can I somehow assist you?

seanpoulter · 2018-06-11T04:16:56Z

Good call on supporting the old default "" value. I don't have any other changes.

The rest is up to you and @orta. ✌️

connectdotz · 2018-07-04T14:26:05Z

src/Settings/index.test.ts

+  it('returns false otherwise', () => {
+    expect(isDefaultPathToJest('')).toBe(false)
+  })
+})


@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...

ok, I will move this test into the normal tests directory in #341

…ommunity#313/Fix-regression-in-support-for-react-scripts Fix regression detecting create-react-app

seanpoulter requested review from orta, ThomasRooney and stephtr May 22, 2018 03:59

stephtr reviewed May 22, 2018

View reviewed changes

ThomasRooney approved these changes May 22, 2018

View reviewed changes

seanpoulter commented May 23, 2018

View reviewed changes

seanpoulter added 2 commits May 22, 2018 22:18

Fix regression detecting create-react-app

44bbfee

Respond to PR comments and expand CRA acronym

ff45916

seanpoulter added the stage: needs review label May 23, 2018

stephtr reviewed May 26, 2018

View reviewed changes

stephtr mentioned this pull request May 26, 2018

Add usage of shell when creating Settings #297

Closed

stephtr mentioned this pull request Jun 10, 2018

Jest version detection fails for global Jest installs #335

Closed

seanpoulter added 2 commits June 11, 2018 00:06

Rename function for clarity

b13b360

Support legacy default "pathToJest" setting

c6c437a

stephtr approved these changes Jun 11, 2018

View reviewed changes

orta merged commit c8c143b into jest-community:master Jul 1, 2018

connectdotz mentioned this pull request Jul 1, 2018

Add usage of shell when gathering Jest settings #340

Merged

connectdotz reviewed Jul 4, 2018

View reviewed changes

legend1202 pushed a commit to legend1202/vscode-jest that referenced this pull request Jun 18, 2023

Merge pull request jest-community#332 from seanpoulter/feature/jest-c…

53f9e3d

…ommunity#313/Fix-regression-in-support-for-react-scripts Fix regression detecting create-react-app

Fix regression detecting create-react-app #332

Fix regression detecting create-react-app #332

Conversation

seanpoulter commented May 22, 2018

coveralls commented May 22, 2018 • edited Loading

Pull Request Test Coverage Report for Build 403

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephtr May 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanpoulter commented May 25, 2018

Choose a reason for hiding this comment

seanpoulter Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephtr May 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephtr commented May 26, 2018

stephtr commented Jun 10, 2018

seanpoulter commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 22, 2018 •

edited

Loading

stephtr May 26, 2018 •

edited

Loading

seanpoulter Jun 11, 2018 •

edited

Loading

stephtr May 26, 2018 •

edited

Loading