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

Slow tests with 9.0.0-next.6 #757

Closed
johncrim opened this issue Jan 25, 2021 · 28 comments · Fixed by #800
Closed

Slow tests with 9.0.0-next.6 #757

johncrim opened this issue Jan 25, 2021 · 28 comments · Fixed by #800

Comments

@johncrim
Copy link
Contributor

johncrim commented Jan 25, 2021

💥 Performance Regression Report

I've spent a lot more time than I'd like to admit upgrading to 9.0.0-next* and getting things working, and trying all different configurations to determine if it's ready to use. This past week was the 3rd time I've attempted to upgrade, but my conclusion is that the performance hit from upgrading to 9.0.0-next is still too big. I'm detailing a bunch of things that I tried here in hopes of both helping the performance effort for this project, and helping others with perf troubleshooting.

In a nutshell, I've found the release version (jest-preset-angular@8.3.1) to yield acceptably fast tests - "I'd like faster, but Jest is the best option for most tests". And I've found 9.0.0-next.6, and earlier 9.0.0-next versions, with isolatedModules: false, unacceptably slow - 7x slower than @8.3.1 for the first run.

Note that 9.0.0-next.6 with isolatedModules: true is FAST - it's what I'd hoped for using Ivy in Jest. Unfortunately we don't want the limitations of typescript compiling with isolatedModules, as there's no other benefit to using isolatedModules that we care about. If isolatedModules resulted in faster Angular build times, or some other similar benefit, it would probably be worth giving up const enum.

Primary request: Is there any way we can use the same jest compilation path that is being used for isolatedModules: true, but without isolatedModules: true in tsc?

Secondary request: Explanation as to why isolatedModules: true is soooo much faster than isolatedModules: false in 9.0.0-next.

Last working version

Worked up to version: 8.3.1

Stopped working in version: 9.0.0-next.3, 9.0.0-next.6

Motivation

The main reason I've been putting so much effort into upgrading to 9.0.0-next.6 is Ivy support: There are a number of benefits (type safety + build perf), but the main reason is that we have code that depends on Ivy behavior, than doesn't exist in ViewEngine (right now we have to run those tests in Karma). Also, it makes sense to test with the same code used in production.

Since it's available, I also decided to try Jest + node ESM support. Since we use ESM (primarily) in prod, and many libraries ship separate code for ESM, let's use the ESM code in tests. And, I was hopeful it would help perf - partly b/c I figured we could use pre-built library code within Jest, reducing Jest's compilation workload.

Upgrade steps

To clarify the changes between these different test runs:

yarn add -D jest-preset-angular@next jest@next

jest.config.js changes :

require('jest-preset-angular/ngcc-jest-processor');

module.exports = {
  preset: 'jest-preset-angular/presets/defaults', // or 'jest-preset-angular/presets/defaults-esm',

setup-jest.ts changes :
Replace import 'jest-preset-angular'; with import 'jest-preset-angular/setup-jest';

Upgrading to ESM

In jest.config.js, use 'jest-preset-angular/presets/defaults-esm'.

Run tests using node --experimental-vm-modules ./node_modules/jest/bin/jest.js .

Performance Tactics

A few of the things I tried to improve Jest performance:

  • Switching to use ESM. It turns out this doesn't help noticeably, probably b/c UMD modules are loaded by Jest's default resolver - for now, at least. It also doesn't achieve my other goal, of using the same library code as we use in production - for now everything run in Jest is UMD (or CommonJS) and ES5.

Note that I could load internal ESM files during Jest compilation by using this in my Jest.config (and equivalent in tsconfig.spec.json):

moduleNameMapper: '^@this/([a-z\-]+)/([a-z\-]+)$': '/dist/$1/fesm2015/this-$1-$2.js'

But all the external dependencies (angular, zone, rxjs) are still loaded as UMD - as noted in #751, this requires a resolver that is ESM aware.

  • Using pre-built library build output instead of source references, to reduce Jest compile workload.

In other words, changing tsconfig.spec.json from:

    "paths": {
      "@this/*": [
        "libs/*/src/public-api.ts"
      ],
    }

to

    "paths": {
      "@this/*": [
        "dist/*"
      ]
    }

and jest.config.js from :

  moduleNameMapper: {
    '^@this/(.*)$': '<rootDir>/libs/$1/src/public-api.ts'
  }

to

  moduleNameMapper: {
    '^@this/(.*)$': '<rootDir>/dist/$1', <-- Linking doesn't work
    'tslib': '<rootDir>/node_modules/tslib/tslib.es6.js'
  }

This didn't work for me because we use some language features that break in ES5 (base class properties), and this linking path combined with jest's default resolver pulls in UMD/ES5 library code. You can work around that for internal libraries using:

  moduleNameMapper: {
	'^@this/([a-z\\-]+)/([a-z\\-]+)$': '<rootDir>/dist/$1/fesm2015/this-$1-$2.js',
    '^@this/([a-z\\-]+)$': '<rootDir>/dist/$1/fesm2015/this-$1.js',
    'tslib': '<rootDir>/node_modules/tslib/tslib.es6.js'
  }

But it will still use UMD/ES5 for external libraries.

Timing data

This is non-scientific, run on an 8 core laptop with SSD.

jest-preset-angular@8.3.1 with jest 26, isolatedModules: false (default) :
for 392 tests, 78 suites
First run (no cache): 62s
2nd run (uses cache): 27s

jest-preset-angular@8.3.1 with jest 26, isolatedModules: true :
for 381 tests, 71 suites (8 suites don't compile)
First run (no cache): 42s
2nd run (uses cache): 32s

jest-preset-angular@9.0.0-next.6 with jest jest@27.0.0-next.2, isolatedModules: false, 'jest-preset-angular/presets/defaults', +ngcc-jest-processor
for 392 tests, 78 suites
First run (no cache): 446s
NOTE: The first test doesn't complete for 115s
2nd run (uses cache): 33s

jest-preset-angular@9.0.0-next.6 with jest jest@27.0.0-next.2, isolatedModules: true, 'jest-preset-angular/presets/defaults'
for 381 tests, 71 suites (8 suites don't compile)
First run (no cache): 43s
2nd run (uses cache): 32s

jest-preset-angular@9.0.0-next.6 with jest jest@27.0.0-next.2, isolatedModules: false, 'jest-preset-angular/presets/defaults-esm'
for 392 tests, 78 suites
First run (no cache): 435s
NOTE: The first test doesn't complete for 117s
2nd run (uses cache): 41s

jest-preset-angular@9.0.0-next.6 with jest jest@27.0.0-next.2, isolatedModules: true, 'jest-preset-angular/presets/defaults-esm'
for 282 tests, 52 suites (27 suites don't compile)
First run (no cache): 41s
2nd run (uses cache): 32s

jest-preset-angular@9.0.0-next.6 with jest jest@27.0.0-next.2, isolatedModules: false, 'jest-preset-angular/presets/defaults-esm', internal library ESM references
for 392 tests, 78 suites
First run (no cache): 258s
NOTE: The first test doesn't complete for 40s
2nd run (uses cache): 40s

jest-preset-angular@9.0.0-next.6 with jest jest@27.0.0-next.2, isolatedModules: true, 'jest-preset-angular/presets/defaults-esm', internal library ESM references
for 287 tests, 55 suites (24 suites don't compile)
First run (no cache): 38s
2nd run (uses cache): 27s

Tests per second for different configurations

Configuration 1st run 2nd run 1st run tests/sec 2nd run tests/sec
8.3.1 62s 27s 6.3 14.5
8.3.1 + isolatedModules 42s 32s 9.1 11.9
9.0 446s 33s 0.9 11.9
9.0 + isolatedModules 43s 32s 8.9 11.9
9.0 + ESM 435s 41s 0.9 9.6
9.0 + ESM + isolatedModules 41s 32s 6.9 8.8
9.0 + ESM + internal library ref 258s 40s 1.5 9.8
9.0 + ESM + isolatedModules + internal library ref 38s 27s 7.6 10.6

Link to repo

I can't expose all our tests, but the setup is very similar to this repo + branch:

https://github.com/johncrim/repro-ivy-jest-preset-angular/tree/bug/inline-directive

envinfo

System:
    OS: Windows

Npm packages:
    jest: 27.0.0-next.2
    jest-preset-angular: 9.0.0-next.6
    typescript: 4.0.5
@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 25, 2021

There are a few things which are different from 8.x.x

  • Switching to the usage of TypeScript Program instead of TypeScript Language Service (with ts-jest)
  • Extra downlevel ctor AST transformer for isolatedModules: false (extra AST transformer = extra cost to walk through AST tree)

isolatedModules: true has been always fast because it is a simple TypeScript Program without module resolution or file reading, which reduces most of the work.

I don't understand exactly about the Primary request

@johncrim
Copy link
Contributor Author

Thank you for the quick reply, @ahnpnl.

I'll try to explain my primary request, but it's relatively open-ended, and may be infeasible - I don't have much understanding of what the preset does. I'm looking for any way to get the simpler + faster code path of isolatedModules: true without actually using isolatedModules: true when calling the typescript compiler.

I looked at this PR, which says:

Use replace-resources transformer for isolatedModules: false.
inline-files and strip-styles are only used for isolatedModules: true.

So, for example, I'm wondering if I can try switching out the transformers, eg use inline-files and strip-styles, or is that likely to be a wasted effort? Or, do you have any other ideas for achieving performance similar to isolatedModules: true.

10x slower is a LOT slower, so I figure it's worth figuring out the root cause and optimizing it.

Thank you again for all your work on this!!

@wtho
Copy link
Collaborator

wtho commented Jan 25, 2021

The performance gain for isolatedModules: true comes mostly from tsc.
The transformers are only used to fill a gap which allows angular syntax to be used with Jest.

The different transformers are almost the same implementation, it's just that we switched to Angular's implementation since that means less code to maintain for us.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 25, 2021

Like I explained above, isolatedModules: true actually uses a simple TypeScript Program behind the public API ts.transpileModule. I have copied the whole implementation of that function from TypeScript source code to be able to hack it to use all possible Angular AST transformers, see https://github.com/thymikee/jest-preset-angular/blob/master/src/compiler/ng-jest-compiler.ts#L179

Short explanation according to what I've experienced and understood from ts.transpileModule

  • Create a simple TypeScript Program.
  • Ignore readFile. This is different from isolatedModules: false where the compiler needs to read files.
  • Skip module resolution (noResolve: true). This is different from isolatedModules: false where module resolution is required to get enough information for type checking as well as compilation.
  • Some other hidden things which I'm not aware of

Most of the hard work in isolatedModules: false is related to read file, module resolution and perhaps some other things which are related to how type checking works.

Currently I only have one solution to improve for isolatedModules: false is skip processing Angular package format umd, which speeds up a bit but not as significantly like isolatedModules: true.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 25, 2021

Besides, there is something about using Program directly vs using Language Service which can result in differences.

I’ve seen that before but no clue why that is different since Language Service is actually a wrapper around Program, more investigation can be done into those differences.

@johncrim
Copy link
Contributor Author

2 more updates on performance data:

On Linux, the slow down between 8.3.1 and 9.0.0-next.6 is much less pronounced: I'm seeing about a 35% slow down on Linux vs a 400%-700% slowdown on Windows. So, that's good news unless you develop on Windows.

Also, skipLibCheck in the tsconfig settings doesn't seem to make a performance difference on 9.0.0-next.6. This is interesting b/c setting it to true does speed up Angular builds, but doesn't seem to make any difference with jest.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 26, 2021

Maybe cache file miss on Windows ? After each time a file is read, it is put into memory. It can be that when getting from memory, the path is Windows path while cached path is different. This can be checked by producing the log file via env variable.

we don’t process .d.ts so skipLibCheck doesn’t see much difference.

@johncrim
Copy link
Contributor Author

johncrim commented Jan 26, 2021

Hmm - I'll dig into file caching on Windows and see what I can find. I've looked at the ts-jest log recently and didn't see anything.

I'm seeing another issue on Windows (haven't tested on Linux) that might be related: When running in --watch mode, file changes are detected (eg the test file rebuilds), but only the original code is run - the updated code is not running. I have to clear the cache and restart to pick up changes. If I run jest not in watch mode, but with file caching enabled, the changes are picked up - so this doesn't appear to be a file cache issue - more an interaction between watch mode and the file cache. But I still see the same issue with --no-cache --watch. I'm not seeing this issue in my simple repro repo that is using the same versions of jest and jest-preset-angular, so my assumption so far is that this --watch issue is an issue on my end.

Not sure if that's related to any perf issues or not, b/c the perf problem is there without --watch mode.

@johncrim
Copy link
Contributor Author

johncrim commented Jan 27, 2021

I used procmon to collect logs for all the system-level File API calls while jest is running with the cache empty. It appears that the files being written to jest-transform-cache are being overwritten about 11 times each, for the same transform output. So, there may be a race condition or something causing that, and if so that could explain the perf degradation, or at least be a place for optimization. I haven't looked at the code yet - but I'd be happy to, if you could point me to where to look.

Here's an example - this is just after the setupFiles have been run, this is during the transformation of tslibes6:

  1. There are 11 attempts to read this file, each by different processes, and the file doesn't exist:
    .jest-cache\jest-transform-cache-89b28e2cd471c78996cc4a492bef085b-27d9edea7a8b986685707af21e2a3650\86\tslibes6_864533d785bff6320e717380559a2504
  2. Then, the first process checks again for the .map file and the cache entry (both not found), and creates these files:
.jest-cache\jest-transform-cache-89b28e2cd471c78996cc4a492bef085b-27d9edea7a8b986685707af21e2a3650\86\tslibes6_864533d785bff6320e717380559a2504.map.2692804696
.jest-cache\jest-transform-cache-89b28e2cd471c78996cc4a492bef085b-27d9edea7a8b986685707af21e2a3650\86\tslibes6_864533d785bff6320e717380559a2504.965846752

writing about 50k to the cache entry.
3. Then, the first process renames the files to:

.jest-cache\jest-transform-cache-89b28e2cd471c78996cc4a492bef085b-27d9edea7a8b986685707af21e2a3650\86\tslibes6_864533d785bff6320e717380559a2504.map
.jest-cache\jest-transform-cache-89b28e2cd471c78996cc4a492bef085b-27d9edea7a8b986685707af21e2a3650\86\tslibes6_864533d785bff6320e717380559a2504

and then the files are closed.
4. Then, another one of the processes tries reading the map file, which succeeds; but then it goes ahead and creates another set of temporary files with different suffixes, eg

.jest-cache\jest-transform-cache-89b28e2cd471c78996cc4a492bef085b-27d9edea7a8b986685707af21e2a3650\86\tslibes6_864533d785bff6320e717380559a2504.map.1427927988
.jest-cache\jest-transform-cache-89b28e2cd471c78996cc4a492bef085b-27d9edea7a8b986685707af21e2a3650\86\tslibes6_864533d785bff6320e717380559a2504.3693383641

and writes the same number of bytes to each file (I can't say for sure that the contents are the same, but it is the same number of bytes for the map file and cache entry), and then renames them to the same files.
5. This happens 9 more times - in other words, there are 11 copies of the cache entry created (all with exactly the same size, and within a second and a half), when 1 would suffice. It appears that this happens with other files too, but I'm not sure whether it's 11 copies for each file.

If I run jest --runInBand, presumably this same problem doesn't occur, but it still startups up quite slowly - eg 19s for the first suite, and most of the first 20 suites take 5-30s each. So this may be a problem, but probably not the only one.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 27, 2021

Ah ha, nice information !! I think if the case of monorepo it is possibly related to this jestjs/jest#10835

Each transform file has a cache key which is provided by https://github.com/kulshekhar/ts-jest/blob/master/src/ts-jest-transformer.ts#L257 Each of the cache key is associated to a transformer’s cache. So the situation with the example of tslib can be that:

  • transformer 1 transforms tslib and caches it as <transformer-1-cache-path>-<transformed-file-cache-key>
  • transformer 2 transforms tslib and caches it as <transformer-2-cache-path>-<transformed-file-cache-key>
  • These are 2 different cache locations on disk and they are not shared.

It might also explain why I didn’t see much difference with my work project since I use single project only.

@johncrim
Copy link
Contributor Author

The project that I've been measuring is a monorepo (multiple libs and apps), but we (currently) run it with a single jest config in the root for the whole repo - that's been fine b/c the perf was good. So it's not the same situation as https://github.com/lexanth/jest-projects-repro , which is the repro repo for jestjs/jest#10835 .

What I'm seeing is that the initial and final cache path and key are the same (all 11 processes create or re-create the same file), the only difference is the suffix used by each process for writing the file before it renames to the same target file. It seems like the 2nd process and later are not properly detecting that the file exists, even though procmon says the first read for the map file succeeded.

I'd assumed that the number of jest worker processes was based on available cores, if not specified.

I'll step through the transformer cache key logic and see what I can figure out. Thank you for the pointer!

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 28, 2021

There is one thing to make clear that tests run in JIT mode, which doesn't actually use Ivy compiler directly but use normal TypeScript Program to transpile ts to js.

The whole implementation is lying in https://github.com/angular/angular-cli/blob/master/packages/ngtools/webpack/src/angular_compiler_plugin.ts#L382 which jest-preset-angular replicates the similar logic.

The issue with Jest always is: transformer is a pure function which is a part of Jest eco system. It is different from Angular CLI where they invoke compilation and webpack before triggering Karma + Jasmine. With Jest transformer, we transform file by file which is passed from jest-runtime while tests are running.

Since transformer in each worker (associated to each CPU thread) doesn't know about the others, it ends up that there is an x amount of TypeScript Program created based on the x amount of worker threads, which results in overhead.

I want to look into the approach of compiling before running tests like Angular CLI does. That approach looks promising.

@johncrim
Copy link
Contributor Author

I have a hypothesis on a way to fix this. I spent a bunch of time yesterday stepping through the code and adding logpoints to see what is going on. Now I understand what jest-preset-angular does :)

It appears that, in the "cache is empty case", the list of NgJestCompiler._rootNames is starting out as an empty array - it doesn't seem to be seeded with all the current test files, even though it looks like there is code that should do that. So each time a new file is transformed, it's not in the existing _rootNames, so it is added to the array, and a new typescript Program is re-created. If the _rootNames could be correctly initialized on startup (to either all the test files, or all the test files filtered by the jest CLI test pattern), then only 1 Program would be created, and I bet the runtime would improve dramatically.

This isn't an issue for isolatedModules: true, but there's no perf issue there.

I will debug why _rootNames isn't correctly initialized on startup, and see if it's something I can figure out how to fix.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 28, 2021

rootNames comes from the setting in tsconfig. Normally, for a test tsconfig, it will include all test files + all custom typings. However, there is an issue to create Program initially with the list of files. The Program will take lots of time to read all those files as well as module resolution which leads to slow startup.

So we took the approach to stimulate incremental compilation. Test files are excluded from startup Program and other files which are not known at startup will be automatically added to Program later, as well as test files. This will reduce I/O on disk which helps for non-SSD users.

Fixing the initial rootNames is not actually the right fix, the main issue is that, each worker thread has its own Program. That significantly increases the workload on CPU. IMO, the best approach is compile ahead of test starts and let Jest transformer reuse the cached compile codes as well as cached file contents.

In general, the approach to work with Jest is file by file processing. isolatedModules: true is a completely different thing. It is a Program which only knows the information of current processing file but doesn't know about other files, that is why it is called "isolated modules"

@johncrim
Copy link
Contributor Author

What you're saying makes sense - thank you for the explanation. I like the idea of compiling ahead of time. The only issue I see with that is avoiding unnecessary compilation.

How about the idea of limiting the scope of a Program's rootNames to just the test file and the setup files. And when the worker moves on to the next test, it passes in the old Program to the new one to presumably re-use some of the previous compilation.

Right now I'm under the impression that the Program is getting recreated many times, and just getting larger and larger, and the old rootNames are no longer needed.

The idea of limiting the Program's scope is probably easier said than done, because I don't see a straightforward way for the NgJestCompiler to know when the root test file has changed (eg the worker has switched from foo.spec.ts to bar.spec.ts).

I'll keep testing to see if I can find any changes that work and make a big perf difference.

@johncrim
Copy link
Contributor Author

johncrim commented Jan 29, 2021

Based on my now slightly improved understanding, the explanation for the 11 parallel + redundant transforms of tslibes6 is:

  • tslibes6 is not a root file - it's pulled in as a transitive dependency of setup-jest.ts
  • each worker is transforming setup-jest.ts b/c it's a setupAfterEnv file

I'm still puzzled why the file cache isn't working - perhaps it's just a race condition. Would be nice to improve, but I don't think it's the critical issue.

@wtho
Copy link
Collaborator

wtho commented Jan 29, 2021

Does the cache still not work when limiting the number of workers to 1 (e. g. using --runInBand)?
If it works, rootNames should contain the files required in many tests to precompile them for the several workers. I am not sure though how jest-preset-angular could guess which files are required by many test suites.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 29, 2021

How about the idea of limiting the scope of a Program's rootNames to just the test file and the setup files. And when the worker moves on to the next test, it passes in the old Program to the new one to presumably re-use some of the previous compilation.

The problem with this approach is: each test file depend on some certain files aka resolved modules. This is the information which is needed for Program to perform type checking as well as AST transformers to perform certain things.

Currently, we already passed the old program into the new program for each processing file passed from jest-runtime to jest-transform, see https://github.com/thymikee/jest-preset-angular/blob/master/src/compiler/ng-jest-compiler.ts#L166 . When an old Program is passed into new Program, all the cached Source File as well as cached files' contents are reused.

An issue is that, in parallel mode, each transformer doesn't talk to each other and therefore they don't share the Source File cache. Files' contents cache is shared because jest-runtime does it.

An idea is that jest-runtime allows transformers to have custom cache and share that cache to other worker threads. Still another issue with that is, sharing instances don't work with serialization in nodejs, just hope that jest-runtime has a way to share that cache to other threads without serialization.

Right now I'm under the impression that the Program is getting recreated many times, and just getting larger and larger, and the old rootNames are no longer needed.

In the implementation of AngularCompilerPlugin of @ngtools/webpack, they do have a way to remove the unnecessary files from root file names. Indeed when Program gets bigger, more memory usage becomes an issue. Unfortunately, I couldn't find a way to integrate the logic of AngularCompilerPlugin to NgJestCompiler because the eco system is different.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 29, 2021

  • Switching to use ESM. It turns out this doesn't help noticeably, probably b/c UMD modules are loaded by Jest's default resolver - for now, at least. It also doesn't achieve my other goal, of using the same library code as we use in production - for now everything run in Jest is UMD (or CommonJS) and ES5.

Check discussion about improving default resolver here jestjs/jest#11034

@johncrim
Copy link
Contributor Author

I tried a few different approaches to improve perf, and these changes worked well for me:

johncrim@23da68d

The change, in a nutshell, is to limit the root files to just the file being transformed plus the non-test roots, instead of accumulating all files that the worker has transformed since testing started. I tested this going back and forth with the current approach, and it makes a significant difference (in my repo 2x faster for first run). I also avoided creating the Program until a file is transformed (just b/c it will always need to be re-created).

And, I turned on incremental: true in the tsconfig if it's not explicitly set - this seems to help a bit (I measured 5% faster), but the benefit is small enough that it could be experimental error.

Also, there's an important change needed for my commit yesterday to fix the --watch problem: Just checking that the SourceFile has changed before updating it. It turns out typescript does a lot of work when SourceFile.update() is called, so there's no point in recompiling it if there's no change.

Here are the timings I'm seeing:

jest-preset-angular@9.0.0-custom with isolatedModules: false, 'jest-preset-angular/presets/defaults-esm'
for 424 tests, 86 suites
First run (no cache): 118s
2nd run (uses cache): 35s

1st run test/sec: 3.6
2nd run tests/sec: 12.1

This puts it 2x as fast (for me) on Windows 1st run with Ivy support, though 8.3 (non Ivy) is still twice as fast as this. And it's as fast as any when the file cache is in place.

I think this could still be a lot better with your pre-compile idea (there's still a lot of waste I'm sure), but for a relatively "easy" tweak on top of all the work you've done, this seems like a good perf improvement.

Let me know if you'd like me to create a PR, or if you just want to take a look. One issue I have is that 2 of the tests are failing, and I'm not sure the right way to fix them. The failures are:

 FAIL  src/__tests__/replace-resources.spec.ts (17.191 s)
  ● Replace resources transformer › with isolatedModules false › should use replaceResources transformer from @angular/compiler-cli with useESM true
 FAIL  src/__tests__/ng-jest-compiler.spec.ts (21.432 s)
  ● NgJestCompiler › with isolatedModule false › should compile codes with useESM true

Neither of the tests report readable errors in the console, but these 2 errors can be found internally:

message:'src/__tests__/__mocks__/app.component.ts(1,27): error TS2792: Cannot find module '@angular/core'. Did you mean to set the 'moduleResolution' option to 'node', or to add aliases to the 'paths' option?\nsrc/__tests__/__mocks__/app.component.ts(3,1): error TS2354: This syntax requires an imported helper but module 'tslib' cannot be found.'
name:'TSError'
stack:''

@ahnpnl
Copy link
Collaborator

ahnpnl commented Jan 30, 2021

Left a few comments on the commit. The approach was almost the same as what I thought of, only problem is that there will be an issue that types change won't trigger re type checking because cache key is the same, see the PR kulshekhar/ts-jest#2167 and its related issues.

Regarding to the errors, I suspect the approach might cause side effects or unknown errors. I will try to test your tar file against my work project.

@johncrim
Copy link
Contributor Author

johncrim commented Jan 30, 2021

Thank you @ahnpnl .

RE the test errors, it is trying to resolve @angular/core and tslib b/c all the mock files are in the rootNames (eg src/__tests__/__mocks__/app.component.ts). But those files were in the root before when the ts.Program was created in the constructor. That doesn't seem problematic to me, but I didn't know the right way to mock them. Also odd that all the other tests passed.

@johncrim
Copy link
Contributor Author

johncrim commented Jan 30, 2021

@ahnpnl - you're correct that when a dependency (eg a component) is updated, tests using the dependency are re-run, but it's still using the old dependency transform output. Ie what kulshekhar/ts-jest#2167 fixed (I think). Hmm.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 4, 2021

FYI, I've made a version which uses ts-jest to compile instead of NgJestCompiler. To test it, first you need to:

  • Download jest-preset-angular.zip
  • Download ts-jest.zip
  • Unzip and copy them to your node_modules. These are ts-jest and jest-preset-angular folders required for your node_modules.
  • Clear Jest cache and run tests

I myself tested and I didn't see any differences comparing to the usage with NgJestCompiler. Note that only test with isolatedModules: false

Another note is: this version requires any types in constructor to be imported without using import type syntax. Apparently there is something different between TypeScript Program (use in NgJestCompiler) vs TypeScript Language Service (use in ts-jest).

The whole experiment is making 9.0.0 work exactly the same like 8.3.2 except that using different AST transformers.

@johncrim
Copy link
Contributor Author

johncrim commented Feb 5, 2021

Your ts-jest compiler works great for me, as long as I don't try to use ESM (which isn't really working anyway, due to the jest resolver). Perf-wise, I'm seeing:

Using:

  • Not running with node --experimental-vm-modules ./node_modules/jest/bin/jest.js
  • Using direct linking (libs/*/src/public-api.ts), not (dist/*)
  • preset: jest-preset-angular/presets/defaults

Perf:
85 suites, 423 test in 61s (1st run)
22s (cached)

When linking to libraries (still no ESM)
85 suites, 423 test in 59s (1st run)
22s (cached)

So, on Windows, this is as fast as 8.3 on first run, and faster than 8.3 when cached. And it's using Ivy (obviously), b/c I have tests that fail on ViewEngine. If this is a viable approach, it definitely addresses this issue.

It's interesting that I didn't see any perf improvement from linking to pre-built libraries. Presumably this is due to the fact that transformation is happening either way.

When I use jest-preset-angular/presets/defaults-esm, I get a bunch of compiler errors about linking errors and interfaces not being importable (similar to what I saw when using isolatedModules: true). Eg:

 Test suite failed to run

    SyntaxError: The requested module './authentication-endpoints' does not provide an export named 'AuthenticationEndpoints'
	
 Test suite failed to run

    linking error, not in local cache	

I didn't debug these errors deeper. I've found that "linking error" means there is an unreported error that I could probably find by running jest in the debugger.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 5, 2021

Interesting about how it resolved the perf issue on your side, that could confirm my theories about something different between using TypeScript LanguageService vs using TypeScript Program. With <9.0.0, we use ts-jest to compile which uses Language Service underlying while 9.0.0 uses Program.

LanguageService is a wrapper around Program but it seems to have other hidden features. According to https://github.com/Microsoft/TypeScript/wiki/Using-the-Language-Service-API, seems like using LanguageService is better with Jest in term of “on demand processing”.

With isolatedModules: true, the experiment doesn’t change the implementation so it still goes to NgJestCompiler which contains the “hacky” solution to use similar set of transformers like isolatedModules: false.

The link error is typical error with ESM, I’m not sure why that happens but I never see it happens on CI, only in my local. I don’t expect any perf on linking because the implementation for ESM in ts-jest is likely the same as NgJestCompiler.

I think it is wise to use ts-jest for isolatedModules: false then.

@ahnpnl
Copy link
Collaborator

ahnpnl commented Feb 12, 2021

Indeed I observed on CI that windows tests are consistently faster with the changes, about 25%.

@johncrim
Copy link
Contributor Author

THANK YOU very much @ahnpnl . For us, this fix keeps jest valuable for Angular development.

On Windows, I'm seeing a 900% speedup when uncached (10m -> 1m) when going from 9.0.0-next.6 to 9.0.0-next.8. When cached, there is a miniscule slow down of 1-2s ( < 1%).

On Linux, the speedup is < 1% (2s) when uncached.

Hopefully tests will become even faster once we can use ESM modules (by avoiding the conversion to CJS).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants