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

JestBuilder not respect globals properties defined in jest.config.js #1059

Closed
kklimczak opened this issue Feb 7, 2019 · 34 comments · Fixed by #1827 or #1896
Closed

JestBuilder not respect globals properties defined in jest.config.js #1059

kklimczak opened this issue Feb 7, 2019 · 34 comments · Fixed by #1827 or #1896
Assignees

Comments

@kklimczak
Copy link

Expected Behavior

Properties defined in globals property inside jest.config.js should be available in jest context.

Current Behavior

At this moment globals is ignored because here JestBuilder set own globals settings so Jest ignores settings from jest.config.js.
This behavior occures when I run tests via ng test. When I run as jest or inside WebStorm IDE tests work fine.

Failure Information (for bugs)

I set enableTsDiagnostics property to enable type-checking inside my tests, but it's not work.

Steps to Reproduce

  1. Init nx workspace with cli,
  2. Add test lib,
  3. Add class (g.e. test) to lib with one param in costructor,
  4. Create spec file for above class and call constructor without param.
  5. Run tests via ng test.

Context

Please provide any relevant information about your setup:

  • version of Nx used - 7.5.1
  • version of Angular CLI used - 7.2.4
  • angular.json configuration
  • version of Angular DevKit used - 0.11.2
  • 3rd-party libraries and their versions -> package.json
  • and most importantly - a use-case that fails

I prepare repo with bug reproduction. In my case I'd like to enable type checking in ts-jest so I need to set enableTsDiagnostics flag to true.

@rweng
Copy link

rweng commented Feb 9, 2019

same problem here. I want to set skipBabel: true because otherwise I have problems with default imports. It works if I insert it directly to the runCLI command line options, here

globals: JSON.stringify({
'ts-jest': {
tsConfigFile: path.relative(builderConfig.root, options.tsConfig)
},
__TRANSFORM_HTML__: true
})

In case anyone else has the exact problem as me: until it is possible to configure the globals in the builder, a workaround is to add the following to the package.json

"jest": {
    "globals": {
      "ts-jest": {
        "skipBabel": true
      }
    }
  },

and then running jest --clearCache

@kklimczak
Copy link
Author

I noticed that when I comment:

globals: JSON.stringify({
'ts-jest': {
tsConfigFile: path.relative(builderConfig.root, options.tsConfig)
},
__TRANSFORM_HTML__: true
})

jest fetch globals from jest.config.js correctly.

@mohyeid
Copy link

mohyeid commented Feb 20, 2019

Same issue I am facing here while trying to migrate to jest-preset-angular alpha-2, some new configurations needed to be set in the globals and the builder override it. globals need to be extensible. #thymikee/jest-preset-angular#217

@kklimczak
Copy link
Author

Instead of defining globals here:

globals: JSON.stringify({
'ts-jest': {
tsConfigFile: path.relative(builderConfig.root, options.tsConfig)
},
__TRANSFORM_HTML__: true
})

cli will generate this lines inside jest.config.ts?

@lonix1
Copy link

lonix1 commented Mar 22, 2019

Hey guys there are many issues lately which seem unrelated but they are - nx isn't playing nice with jest-preset-angular and so tests don't work.

Can someone share a working config: jest.config.js and tsconfig.spec.json, etc.?

@FrozenPandaz
Copy link
Collaborator

I'd like to get this to work as well.

The reason it does not work is that jestCLI node api seems to take a stringified json which means the whole globals input is overwritten. We only need to overwrite the tsConfig property of ts-jest. Does anybody have an idea of how this could work?

@lonix1
Copy link

lonix1 commented Mar 22, 2019

Hey @FrozenPandaz Thanks for looking into this, we've been going round in circles for days. Some of the stuff we've tried:

  1. I had a hunch nx was using the old APIs, so I did this:
  globals: {
    'ts-jest': {
      tsConfig: './tsconfig.spec.json',
      tsConfigFile: './tsconfig.spec.json',
    }
  }

I thought that would force it to work with the old AND new APIs, but sadly that doesn't work either. So it's probably not an API thing.

  1. I tried the workaround here to use a common config for all apps in the workspace. But that doesn't help either. It gets us some of the way, but something under the covers is still overriding config. So we swap one set of errors for another.

  2. I tried adding the config to package.json and then to jest.config.js in the hope it was a race condition in the way the config is applied, but that didn't work.

@lonix1
Copy link

lonix1 commented Mar 22, 2019

The stringification may not be a problem - see the migration notes for jest-preset-angular.

The globals should be overridable, I think that's how the preset does it (override/extend rather than replace).

@mohyeid
Copy link

mohyeid commented Mar 22, 2019

@lonix1
What you can do to get this work in your local is open the jest.builder.js file and add the required configuration there. Builder will keep overriding your configuration if you place them in the jest.config file. So to persist them go to node_modules@nrwl\builders\src\jest\jest.builder.js and change the globals to the following:

globals: JSON.stringify({
        'ts-jest': {
          tsConfigFile: path.relative(builderConfig.root, options.tsConfig),
		  stringifyContentPathRegex: '\\.html$',
		  astTransformers: ['jest-preset-angular/InlineHtmlStripStylesTransformer']
        },
        __TRANSFORM_HTML__: true

If you want this to work in your build server, this is another story as you have to download the tar file for @nrwl/builders and patch it, then use your local version for installation. Let me know if you need help with this.

@lonix1
Copy link

lonix1 commented Mar 22, 2019

@mohyeid Thanks... Do you mean this line? Nope that doesn't work for me I still can't get tests to run. Maybe we are using different versions, or different config files. I also tried commenting out the stuff mentioned in the posts above.

Hopefully if someone gets this to work, we can get a set of working configs that we can look at. Or a minimal sample repo.

@mohyeid
Copy link

mohyeid commented Mar 22, 2019

@lonix1
I am not sure what version you are in, but the line you sent is off master branch which seems to have the fix that we are trying to do. As I am in 7.1.1 this code was not there and this is why I have to patch it.

@lonix1
Copy link

lonix1 commented Mar 23, 2019

I'm on the latest version.

npx nx --version     # 7.7.2

Version 7.1.1 that you're using is "ancient" in that many bugs have been fixed since then. Is there a reason you're still using it? Anyways, unfortunately, even using the latest doesn't resolve the issue. :(

@kklimczak
Copy link
Author

@FrozenPandaz, did you try to move this config to generated jest.config.ts?

@FrozenPandaz
Copy link
Collaborator

I have not, but have some thoughts:

Caveats:

  • There might be some other things you might want to control from the CLI such as ts-jest diagnostics.
    • Maybe you want to have typechecking in CI but not dev 🤷‍♀️
  • You probably do not need to pass the tsConfigPath after doing this.
    • Removing the tsConfigPath altogether is fine. Sometimes it might be useful to be able to have references from the test target config to the tsconfig file so that we can run some migrations on them but.. I guess we can live without this?

Alternatives:

  • Maybe we can require in the config file and use the globals defined there.
    • Does it always export an object directly?
  • Maybe we can let the user pass in globals and the config will be extended.

@mrm1st3r
Copy link

Being able to modify ts-jest.diagnostics at all would be really important for me.

After migrating from Karma, there was an incorrect test running successfully, which definitely shouldn't.
The cause was something like this (where jasmine accepted n params and jest only 1):
expect([1]).toContain(1, 2);
resulting only in log output on the first run and cached afterwards:
ts-jest[ts-compiler] (WARN) TypeScript diagnostics ... error TS2554: Expected 1 arguments, but got 2.

Currently, I find no possibility to set ts-jest.diagnostics.warnOnly=false on v7.8.1

@4kochi
Copy link

4kochi commented Jul 5, 2019

Are there any plans in future version of nx to make the ts-jest options configurable? Or would you accept a PR for this behavior?

@alfaproject
Copy link
Contributor

I ended up adding this patch using patch-package:

diff --git a/node_modules/@nrwl/jest/src/builders/jest/jest.impl.js b/node_modules/@nrwl/jest/src/builders/jest/jest.impl.js
index 1f2e0e5..4d6a7a5 100644
--- a/node_modules/@nrwl/jest/src/builders/jest/jest.impl.js
+++ b/node_modules/@nrwl/jest/src/builders/jest/jest.impl.js
@@ -18,7 +18,7 @@ function run(options, context) {
         // Typechecking wasn't done in Jest 23 but is done in 24. This makes errors a warning to amend the breaking change for now
         // Remove for v8 to fail on type checking failure
         diagnostics: {
-            warnOnly: true
+            warnOnly: false
         }
     };
     // TODO: This is hacky, We should probably just configure it in the user's workspace

I realise that it was a breaking change but v8 is now out and I really can't wait. We use TS for a reason (:

@ZachJW34
Copy link
Contributor

I am interested in this issue as well. At my company, we recently added the noUnusedLocals and noUnusedParameters flags to our application's tsconfig and expect our builds to fail if these rules are violated. For consistency, our spec tests should also fail and for that we need the globals options of the jest config to be respected. I believe we can solve this issue by:

  1. Let the jest.config.js contain all the options for running jest rather than adding the options in the builder (tsConfig, diagnostics, stringifyContentPathRegex, astTransformers, setupTestFrameworkScriptFile). The options can be added to the jest config via schematics.
  2. Add options to the jest builder for overriding some/all of these options for CI purposes if this is behavior is needed. An example would be overriding warnOnly to true if one did not want this to fail on CI.

With these changes, you could even invoke jest directly as the jest config would have all the information needed to run.

If this seems like a good approach let me know and I can make a PR implementing these.

@vsavkin vsavkin added this to the next milestone Sep 2, 2019
@vsavkin vsavkin assigned jdpearce and unassigned FrozenPandaz Sep 2, 2019
@jdpearce
Copy link
Collaborator

jdpearce commented Sep 9, 2019

Having been assigned to this, I'd like to get some clarity on the right direction to head with the fix.

I'm inclined to agree with @ZachJW34 that the jest.config.js in each project should contain the base configuration options and that the builder config (in angular.json/workspace.json) should contain overrides which in the case of something like globals we can merge.

@FrozenPandaz (or anyone invested in this) - can you see any reason this wouldn't work?

I'll admit I'm fairly new to this project so there may be nuances I've missed. One thing that keeps needling at me is that the builder config is in JSON but the config file is JS so the overrides couldn't have the same potential (e.g. no functions) which suggests a global jest.config.js might be an idea, but maybe that's something to raise in another issue...? 🤷‍♀️

Update : (I'm not sure I should post when I haven't woken up yet. 🤪)

  • The JSON issue isn't an issue as the output of jest.config.js has to be JSON serialisable anyway
  • Workspace options are literally overrides anyway
  • It's Jest itself that does the overriding.

The more I look at this issue, the more I wonder whether this shouldn't be a bug/feature request against Jest itself...

jdpearce added a commit to jdpearce/nx that referenced this issue Sep 12, 2019
jdpearce added a commit that referenced this issue Sep 16, 2019
@4kochi
Copy link

4kochi commented Sep 26, 2019

Hi @jdpearce , thx for the PR. I was waiting for this feature since a long time. Sadly there is still a small problem.

Since the property ts-jest is an object, the Object.assign() does not work as expected. Because it overwrites the property ts-jest if it was set before, instead of making a merge. You can check the example below.

const globals = {
  'ts-jest': { diagnostics: false, isolatedModules: true },
}

const defaultConfig = {
  tsConfig: '../tsconfig.spec.json',
  stringifyContentPathRegex: '\\.(html|svg)$',
  astTransformers: ['jest-preset-angular/InlineHtmlStripStylesTransformer']
}

// does not work 
const mergeBroken = Object.assign(globals, {
  'ts-jest': defaultConfig
});

// works 
const merge = Object.assign(globals, {
  'ts-jest':  Object.assign(defaultConfig, globals['ts-jest'] || {})
});

@jrista
Copy link
Contributor

jrista commented Sep 29, 2019

It does appear as though this bug is still in full force. Because the "overrides" from angular.json are overwriting the entire ts-jest property on globals with the Object.assign, nothing you put into jest.config.js takes effect.

The current approach is a brute-force override, rather than a merger of the two ts-jest objects. Instead of replacing the ts-jest property on the globals, you need to spread both the original as well as the overrides together to create a merged ts-jest config first.

So, DON'T do this:

  // merge the jestConfig globals with our 'ts-jest' override
  const jestConfig: { globals: any } = require(options.jestConfig);
  const globals = jestConfig.globals || {};
  Object.assign(globals, {
    'ts-jest': tsJestConfig
  });

DO this:

  // merge the jestConfig globals with our 'ts-jest' override
  const jestConfig: { globals: any } = require(options.jestConfig);
  const globals = jestConfig.globals || {};
  Object.assign(globals, {
    'ts-jest': {
        ...(globals['ts-jest'] || {}),
        ...tsJestConfig
    }
  });

I have made this small change to the jest builder code in my node_modules, and it seems to work.

@evtk
Copy link

evtk commented Oct 4, 2019

@jrista @jdpearce waiting for this feature, so I have just update to Nx 8.6.0 but this still doesn't seem to work.

This is the globals section in my config in jest.config.js in the root of my repo, watch the isolatedModules.

  globals: {
    'ts-jest': {
      tsConfig: '<rootDir>/tsconfig.spec.json',
      stringifyContentPathRegex: '\\.html$',
      astTransformers: [
        require.resolve('jest-preset-angular/InlineHtmlStripStylesTransformer')
      ],
      isolatedModules: true
    }
  },

now when logging what is coming inside of the jest.imp.js file for a library 'shared-utils'

  const jestConfig = require(options.jestConfig);
  console.log(jestConfig)

gives me:

{ name: 'shared-utils',
  preset: '../../../jest.config.js',
  coverageDirectory: '../../../reports/coverage/libs/shared/utils',
  snapshotSerializers:
   [ 'jest-preset-angular/AngularSnapshotSerializer.js',
     'jest-preset-angular/HTMLCommentSerializer.js' ] 
}

This is the jest.config.js file in the root of this library. Which has a preset on the root jest.config.js. But I don't see any code which merges this root jest.config.js into the jestConfig.

Jest has a normalize file which contains logic to resolve the preset property in jest.config.js. Shouldn't this be applied into the builder to make this work?

https://github.com/facebook/jest/blob/0df9981ef7836bc1eae31a01a3744dc461a25212/packages/jest-config/src/normalize.ts

@jrista
Copy link
Contributor

jrista commented Oct 6, 2019

@evtk: Looking at your examples there...it looks like you are logging the jestConfig before it has been merged.

The code I fixed used to look like this:

  const jestConfig: { globals: any } = require(options.jestConfig);
  const globals = jestConfig.globals || {};
  Object.assign(globals, {
    'ts-jest': tsJestConfig
  });

I updated it to the following, but I've marked where you are (I assume) logging:

  const jestConfig: { globals: any } = require(options.jestConfig);
  // *** YOU ARE LOGGING HERE ***
  const globals = jestConfig.globals || {};
  Object.assign(globals, {
    'ts-jest': {
      ...(globals['ts-jest'] || {}),
      ...tsJestConfig
    }
  });

So you are logging before the fix actually merges your custom globals config with the defaults that the jest builder grabs from config. The key to know if you are using the latest code is to simply check if you have my fix or not. If you do not have my fix, which is the double spread there, then it may just be that a version of the nrwl builder for jest that has the fix has not yet been published.

@evtk
Copy link

evtk commented Oct 6, 2019

@jrista thanks for checking in. I do have the updated code it seems. Now I'm logging below:

  // merge the jestConfig globals with our 'ts-jest' override
  const jestConfig = require(options.jestConfig);
  const globals = jestConfig.globals || {};
  Object.assign(globals, {
    'ts-jest': {
      ...(globals['ts-jest'] || {}),
      ...tsJestConfig
    }
  });
  console.log(globals);

but logging this, still doesn't give me the isolatedModules: true configuration which I have present in my jest.config.js file in the root of the angular repo.

{ 'ts-jest':
   { tsConfig:
      '**removed personal info***/libs/shared/utils/tsconfig.spec.json',
     stringifyContentPathRegex: '\\.(html|svg)$',
     astTransformers: [ 'jest-preset-angular/InlineHtmlStripStylesTransformer' ]
  }
 }

Could you try this out yourself? Am I missing some piece of the puzzle? :)

@IDonut
Copy link

IDonut commented Oct 7, 2019

I think I'm seeing the same issue as @evtk although I may be misunderstanding how this is supposed to work.

It looks like the preset defined in each library's jest.config file is not respected. If I define my global override in my library config everything works perfectly but it's not picked up if the override is defined in the root config file.

I've got 40+ libraries so I'd rather not have to update the config in each library.

@jdpearce
Copy link
Collaborator

jdpearce commented Oct 7, 2019

Hi folks. I am looking at this now and it seems to me that this is technically a Jest problem. Jest is afaict responsible for loading up the preset file and merging that with the local project options. All it does is what we were doing initially :

https://github.com/facebook/jest/blob/0df9981ef7836bc1eae31a01a3744dc461a25212/packages/jest-config/src/normalize.ts#L142-L154

There are some special cases, but I believe if you put something in globals, this means it will always get overridden (I have just tested this in a non Nx workspace and it does seem to be the case)

Is it worth raising an issue against Jest itself? Or maybe someone could create a PR similar to the one @jrista raised for Nx?

To fix this in Nx we would have to do Jest's work for it, which is feasible, but seems like the wrong way to go, imo.

@jdpearce
Copy link
Collaborator

jdpearce commented Oct 8, 2019

I raised an issue with Jest and created a PR to resolve it. If anyone wants to watch how that goes, they're here : jestjs/jest#9026

@IDonut
Copy link

IDonut commented Oct 8, 2019

Brilliant. Thanks for doing that. Much appreciated.

@jrista
Copy link
Contributor

jrista commented Oct 9, 2019

If Jest itself is doing something to override, then that might still be a problem. I did update the unit tests for nrwl/jest builder when I made the fix, so the builder itself should be properly merging globals config (at least, whatever such config the builder works with).

It seems to work for my one project locally, but...I don't think I actually have a situation where a child jest.config.js has globals as well as the root jest.config.js. So I may not actually have encountered the scenario @evtk and @IDonut are experiencing.

@jdpearce
Copy link
Collaborator

jdpearce commented Oct 28, 2019

The PR I raised with Jest has been merged! Hopefully this will solve any problems people are having with the preset globals being clobbered.

@evtk - I'm not sure when Jest will release the change, but please update and try it out when they do!

jestjs/jest#9027 (comment)

Doginal pushed a commit to Doginal/nx that referenced this issue Oct 30, 2019
Doginal pushed a commit to Doginal/nx that referenced this issue Oct 30, 2019
@demisx
Copy link

demisx commented Dec 1, 2019

I have this global setting added to the workspace root jest.config.js to disable diagnostics and it has not effect. It works fine if I move it to the lib's jest.config.js, though. Am I missing something?

// In <workspace-roo>/jest.config.js
module.exports = {
  testMatch: ['**/+(*.)+(spec).+(ts|js)?(x)'],
  ...
  globals: {
    'ts-jest': {
      diagnostics: false,
    },
  },
}
$ npx nx report
 @nrwl/angular : Not Found
  @nrwl/cli : 8.8.3
  @nrwl/cypress : Not Found
  @nrwl/eslint-plugin-nx : Not Found
  @nrwl/express : Not Found
  @nrwl/jest : 8.8.3
  @nrwl/linter : 8.8.3
  @nrwl/nest : 8.8.3
  @nrwl/next : Not Found
  @nrwl/node : 8.8.3
  @nrwl/react : Not Found
  @nrwl/schematics : Not Found
  @nrwl/tao : 8.8.3
  @nrwl/web : Not Found
  @nrwl/workspace : 8.8.3
  typescript : 3.5.3

@jdpearce
Copy link
Collaborator

jdpearce commented Dec 2, 2019

Hi @demisx, Jest hasn't released a new version with the fix for that problem yet. You'll have to keep an eye on their release schedule to know when it will be out. We should be updating our dependency shortly after they release.

@evtk
Copy link

evtk commented Mar 22, 2020

I can report back that after jest has released this fix, it now finally works as we all expected. Thanks for assistance!

@github-actions
Copy link

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.