nrwl / nx

Smart Monorepos · Fast CI
https://nx.dev
MIT License
23.27k stars 2.32k forks source link

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

Closed kklimczak closed 5 years ago

kklimczak commented 5 years ago

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:

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 commented 5 years ago

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

https://github.com/nrwl/nx/blob/2ce53ea2f82f134269aff5d5955680743df00e37/packages/builders/src/jest/jest.builder.ts#L50-L55

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 commented 5 years ago

I noticed that when I comment:

https://github.com/nrwl/nx/blob/2ce53ea2f82f134269aff5d5955680743df00e37/packages/builders/src/jest/jest.builder.ts#L50-L55

jest fetch globals from jest.config.js correctly.

mohyeid commented 5 years ago

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. #https://github.com/thymikee/jest-preset-angular/issues/217

kklimczak commented 5 years ago

Instead of defining globals here: https://github.com/nrwl/nx/blob/2ce53ea2f82f134269aff5d5955680743df00e37/packages/builders/src/jest/jest.builder.ts#L50-L55 cli will generate this lines inside jest.config.ts?

lonix1 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

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

  3. 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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

FrozenPandaz commented 5 years ago

I have not, but have some thoughts:

Caveats:

Alternatives:

mrm1st3r commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.

jdpearce commented 5 years ago

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 more I look at this issue, the more I wonder whether this shouldn't be a bug/feature request against Jest itself...

4kochi commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

IDonut commented 4 years ago

Brilliant. Thanks for doing that. Much appreciated.

jrista commented 4 years ago

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 commented 4 years ago

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!

https://github.com/facebook/jest/pull/9027#event-2745935694

demisx commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

github-actions[bot] commented 1 year ago

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.