Skip to content

Repo: Split up parse.test.ts test to be more parallelizeable #6025

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.

Sign up for GitHub

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

Jump to bottom
Closed
JoshuaKGoldberg opened this issue Nov 17, 2022 · 14 comments · Fixed by #6092
Closed

Repo: Split up parse.test.ts test to be more parallelizeable #6025

JoshuaKGoldberg opened this issue Nov 17, 2022 · 14 comments · Fixed by #6092
Labels
accepting prs Go ahead, send a pull request that resolves this issue good first issue Good for newcomers repo maintenance things to do with maintenance of the repo, and not with code/docs

Comments

@JoshuaKGoldberg
Copy link
Member

Suggestion

Within packages/typescript-estree, parse.test.ts takes about ~11 seconds on average on my M2 Macbook Air. That's quite a lot of time to spend on one test file.

I set the slowTestThreshold to 0 and ran it locally. Here are the tests that took >100ms:

parseAndGenerateServices
  preserveNodeMaps
    ✓ should preserve node maps by default for parseAndGenerateServices() (666 ms)
    project includes
      ✓ doesn't error for matched files (484 ms)
  invalid project error messages
    ✓ throws when non of multiple projects include the file (507 ms)
  projectFolderIgnoreList
    ✓ ignores nothing when given nothing (897 ms)
    ✓ ignores a folder when given a string glob (431 ms)
  moduleResolver
    when file is in the project
      ✓ returns error if __PLACEHOLDER__ can not be resolved (1998 ms)
      ✓ resolves __PLACEHOLDER__ correctly (1903 ms)
    when file is not in the project and createDefaultProgram=true
      ✓ returns error because __PLACEHOLDER__ can not be resolved (1607 ms)
      ✓ resolves __PLACEHOLDER__ correctly (1740 ms)

Proposal: let's move each of the groups inside parseAndGenerateServices to their own test file? For example, parse.moduleResolver.test.ts? That way test runners able to parallelize test running across different cores will be able to make these tests run more in parallel. I applied a similar change in #4599.

Full output from yarn jest lib/parse.test.ts --verbose ```plaintext PASS tests/lib/parse.test.ts (10.879 s) parseWithNodeMaps() basic functionality ✓ should parse an empty string (6 ms) ✓ parse() should be the same as parseWithNodeMaps().ast (5 ms) ✓ should simple code (3 ms) modules ✓ should have correct column number when strict mode error occurs (1 ms) general ✓ output tokens, comments, locs, and ranges when called with those options (2 ms) ✓ output should not contain loc (1 ms) ✓ output should not contain range (1 ms) non string code ✓ should correctly convert code to a string for parse() (1 ms) ✓ should correctly convert code to a string for parseAndGenerateServices() (3 ms) loggerFn should be propagated to ast-converter ✓ output tokens, comments, locs, and ranges when called with those options (1 ms) parseAndGenerateServices errorOnTypeScriptSyntacticAndSemanticIssues ✓ should throw on invalid option when used in parseWithNodeMaps (8 ms) ✓ should not throw when used in parseAndGenerateServices (24 ms) ✓ should error on invalid code (2 ms) preserveNodeMaps ✓ should not impact the use of parse() (3 ms) ✓ should not impact the use of parseWithNodeMaps() (2 ms) ✓ should preserve node maps by default for parseAndGenerateServices() (666 ms) should preserve node maps for parseAndGenerateServices() when option is `true`, regardless of `project` config ✓ without project (1 ms) ✓ with project (1 ms) should not preserve node maps for parseAndGenerateServices() when option is `false`, regardless of `project` config ✓ without project ✓ with project isolated parsing ✓ should parse .js file - without JSX content - parserOptions.jsx = false (1 ms) ✓ should parse .js file - without JSX content - parserOptions.jsx = true (3 ms) ✓ should parse .js file - with JSX content - parserOptions.jsx = false (4 ms) ✓ should parse .js file - with JSX content - parserOptions.jsx = true (2 ms) ✓ should parse .jsx file - without JSX content - parserOptions.jsx = false (4 ms) ✓ should parse .jsx file - without JSX content - parserOptions.jsx = true (1 ms) ✓ should parse .jsx file - with JSX content - parserOptions.jsx = false (1 ms) ✓ should parse .jsx file - with JSX content - parserOptions.jsx = true (1 ms) ✓ should parse .ts file - without JSX content - parserOptions.jsx = false ✓ should parse .ts file - without JSX content - parserOptions.jsx = true (1 ms) ✓ should parse .ts file - with JSX content - parserOptions.jsx = false (2 ms) ✓ should parse .ts file - with JSX content - parserOptions.jsx = true (1 ms) ✓ should parse .tsx file - without JSX content - parserOptions.jsx = false (1 ms) ✓ should parse .tsx file - without JSX content - parserOptions.jsx = true (1 ms) ✓ should parse .tsx file - with JSX content - parserOptions.jsx = false (2 ms) ✓ should parse .tsx file - with JSX content - parserOptions.jsx = true (1 ms) ✓ should parse .vue file - without JSX content - parserOptions.jsx = false (1 ms) ✓ should parse .vue file - without JSX content - parserOptions.jsx = true (1 ms) ✓ should parse .vue file - with JSX content - parserOptions.jsx = false ✓ should parse .vue file - with JSX content - parserOptions.jsx = true (1 ms) ✓ should parse .json file - without JSX content - parserOptions.jsx = false (1 ms) invalid file error messages project includes ✓ doesn't error for matched files (484 ms) ✓ errors for not included files (2 ms) "parserOptions.extraFileExtensions" is empty ✓ should not error ✓ the extension does not match "parserOptions.extraFileExtensions" is non-empty ✓ invalid extension ✓ the extension does not match the extension matches ✓ the file is included (3 ms) ✓ the file isn't included (1 ms) ✓ duplicate extension invalid project error messages ✓ throws when non of multiple projects include the file (507 ms) debug options ✓ shouldn't turn on debugger if no options were provided (1 ms) ✓ should turn on eslint debugger (1 ms) ✓ should turn on typescript-eslint debugger ✓ should turn on both eslint and typescript-eslint debugger (1 ms) ✓ should turn on typescript debugger projectFolderIgnoreList ✓ ignores nothing when given nothing (897 ms) ✓ ignores a folder when given a string glob (431 ms) moduleResolver when file is in the project ✓ returns error if __PLACEHOLDER__ can not be resolved (1998 ms) ✓ throws error if moduleResolver can not be found (2 ms) ✓ resolves __PLACEHOLDER__ correctly (1903 ms) when file is not in the project and createDefaultProgram=true ✓ returns error because __PLACEHOLDER__ can not be resolved (1607 ms) ✓ resolves __PLACEHOLDER__ correctly (1740 ms) ```
@JoshuaKGoldberg JoshuaKGoldberg added triage Waiting for team members to take a look repo maintenance things to do with maintenance of the repo, and not with code/docs good first issue Good for newcomers labels Nov 17, 2022
@Beraliv
Copy link
Contributor

Beraliv commented Nov 19, 2022

Hey @JoshuaKGoldberg!

Can I help you with this one? It looks straight-forward

@JoshuaKGoldberg
Copy link
Member Author

Awesome, yes please @Beraliv! 🙌

I hope and think it should be straightforward, yes. Cut and pasting tests from one file to several. We'll see. 🤞

@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Nov 19, 2022
@Beraliv
Copy link
Contributor

Beraliv commented Nov 19, 2022

Quick question about running a specific jest file for eslint-plugin package: when I want to run yarn jest lib/parse.test.ts within packages/eslint-plugin, I get an error that no tests found:

➜ yarn jest lib/parse.test.ts
yarn run v1.22.19
$ /Users/beraliv/Documents/Code/typescript-eslint/node_modules/.bin/jest lib/parse.test.ts
No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In /Users/beraliv/Documents/Code/typescript-eslint/packages/scope-manager
  887 files checked.
  testMatch:  - 0 matches
  testPathIgnorePatterns: /node_modules/ - 887 matches
  testRegex: ./tests/.+\.test\.ts$, ./tests/.+\.spec\.ts$ - 34 matches
Pattern: lib/parse.test.ts - 0 matches
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I've tried yarn jest packages/typescript-estree/tests/lib/parse.test.ts from typescript-eslint itself, didn't work for me as well

➜ yarn jest packages/typescript-estree/tests/lib/parse.test.ts
yarn run v1.22.19
$ /Users/beraliv/Documents/Code/typescript-eslint/node_modules/.bin/jest packages/typescript-estree/tests/lib/parse.test.ts
No tests found, exiting with code 1
Run with `--passWithNoTests` to exit with code 0
In /Users/beraliv/Documents/Code/typescript-eslint/packages/scope-manager
  887 files checked.
  testMatch:  - 0 matches
  testPathIgnorePatterns: /node_modules/ - 887 matches
  testRegex: ./tests/.+\.test\.ts$, ./tests/.+\.spec\.ts$ - 34 matches
Pattern: packages/typescript-estree/tests/lib/parse.test.ts - 0 matches
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

However if I run let's say yarn jest packages/scope-manager/tests/eslint-scope/es6-class.test.ts, it's working as expected

Is it something specific that needs to be done for eslint-plugin only? Can you please help me with it?

@bradzacher
Copy link
Member

The best place to run a test is within the relevant package's folder.

So try this:

cd packages/typescript-estree
yarn jest /parse.test.ts

@Beraliv
Copy link
Contributor

Beraliv commented Nov 20, 2022

The best place to run a test is within the relevant package's folder.

So try this:

cd packages/typescript-estree
yarn jest /parse.test.ts

I used the wrong folder, my bad 😅 Thank you for your help!

@Beraliv Beraliv mentioned this issue Nov 20, 2022
Closed
3 tasks
@Beraliv
Copy link
Contributor

Beraliv commented Nov 20, 2022 •
edited
Loading

When I split parseAndGenerateServices into these files (see below), I see degradation of performance from 30.375 to 31.100 (+2.39%) in average for 5 runs on my Macbook Air M1.

  1. moduleResolver
  2. projectFolderIgnoreList
  3. invalid project error messages
  4. preserveNodeMaps
  5. rest of tests

For more information, please check #6049

I have another suggestion: maybe it makes sense to split only moduleResolver as most of the time (14s out of 22s, or 63.6%, see the spoiler with my run below) was spent on it?

Output on Macbook Air M1 (main branch) 
 PASS  tests/lib/parse.test.ts (22.151 s)
  parseWithNodeMaps()
    basic functionality
      ✓ should parse an empty string (10 ms)
      ✓ parse() should be the same as parseWithNodeMaps().ast (6 ms)
      ✓ should simple code (2 ms)
    modules
      ✓ should have correct column number when strict mode error occurs (2 ms)
    general
      ✓ output tokens, comments, locs, and ranges when called with those options (3 ms)
      ✓ output should not contain loc (1 ms)
      ✓ output should not contain range (2 ms)
    non string code
      ✓ should correctly convert code to a string for parse() (1 ms)
      ✓ should correctly convert code to a string for parseAndGenerateServices() (4 ms)
    loggerFn should be propagated to ast-converter
      ✓ output tokens, comments, locs, and ranges when called with those options (2 ms)
  parseAndGenerateServices
    errorOnTypeScriptSyntacticAndSemanticIssues
      ✓ should throw on invalid option when used in parseWithNodeMaps (13 ms)
      ✓ should not throw when used in parseAndGenerateServices (40 ms)
      ✓ should error on invalid code (2 ms)
    preserveNodeMaps
      ✓ should not impact the use of parse() (4 ms)
      ✓ should not impact the use of parseWithNodeMaps() (2 ms)
      ✓ should preserve node maps by default for parseAndGenerateServices() (1285 ms)
      should preserve node maps for parseAndGenerateServices() when option is `true`, regardless of `project` config
        ✓ without project (1 ms)
        ✓ with project (2 ms)
      should not preserve node maps for parseAndGenerateServices() when option is `false`, regardless of `project` config
        ✓ without project (1 ms)
        ✓ with project (1 ms)
    isolated parsing
      ✓ should parse .js file - without JSX content - parserOptions.jsx = false (2 ms)
      ✓ should parse .js file - without JSX content - parserOptions.jsx = true (2 ms)
      ✓ should parse .js file - with JSX content - parserOptions.jsx = false (6 ms)
      ✓ should parse .js file - with JSX content - parserOptions.jsx = true (2 ms)
      ✓ should parse .jsx file - without JSX content - parserOptions.jsx = false (2 ms)
      ✓ should parse .jsx file - without JSX content - parserOptions.jsx = true (1 ms)
      ✓ should parse .jsx file - with JSX content - parserOptions.jsx = false (2 ms)
      ✓ should parse .jsx file - with JSX content - parserOptions.jsx = true (1 ms)
      ✓ should parse .ts file - without JSX content - parserOptions.jsx = false (1 ms)
      ✓ should parse .ts file - without JSX content - parserOptions.jsx = true (2 ms)
      ✓ should parse .ts file - with JSX content - parserOptions.jsx = false (3 ms)
      ✓ should parse .ts file - with JSX content - parserOptions.jsx = true (1 ms)
      ✓ should parse .tsx file - without JSX content - parserOptions.jsx = false (2 ms)
      ✓ should parse .tsx file - without JSX content - parserOptions.jsx = true (1 ms)
      ✓ should parse .tsx file - with JSX content - parserOptions.jsx = false (2 ms)
      ✓ should parse .tsx file - with JSX content - parserOptions.jsx = true (2 ms)
      ✓ should parse .vue file - without JSX content - parserOptions.jsx = false (1 ms)
      ✓ should parse .vue file - without JSX content - parserOptions.jsx = true (1 ms)
      ✓ should parse .vue file - with JSX content - parserOptions.jsx = false (1 ms)
      ✓ should parse .vue file - with JSX content - parserOptions.jsx = true (2 ms)
      ✓ should parse .json file - without JSX content - parserOptions.jsx = false (2 ms)
    invalid file error messages
      project includes
        ✓ doesn't error for matched files (918 ms)
        ✓ errors for not included files (3 ms)
      "parserOptions.extraFileExtensions" is empty
        ✓ should not error (1 ms)
        ✓ the extension does not match (1 ms)
      "parserOptions.extraFileExtensions" is non-empty
        ✓ invalid extension (1 ms)
        ✓ the extension does not match (1 ms)
        the extension matches
          ✓ the file is included (4 ms)
          ✓ the file isn't included
          ✓ duplicate extension
    invalid project error messages
      ✓ throws when non of multiple projects include the file (847 ms)
    debug options
      ✓ shouldn't turn on debugger if no options were provided (1 ms)
      ✓ should turn on eslint debugger
      ✓ should turn on typescript-eslint debugger
      ✓ should turn on both eslint and typescript-eslint debugger (1 ms)
      ✓ should turn on typescript debugger (1 ms)
    projectFolderIgnoreList
      ✓ ignores nothing when given nothing (1826 ms)
      ✓ ignores a folder when given a string glob (840 ms)
    moduleResolver
      when file is in the project
        ✓ returns error if __PLACEHOLDER__ can not be resolved (3734 ms)
        ✓ throws error if moduleResolver can not be found (4 ms)
        ✓ resolves __PLACEHOLDER__ correctly (3687 ms)
      when file is not in the project and createDefaultProgram=true
        ✓ returns error because __PLACEHOLDER__ can not be resolved (3107 ms)
        ✓ resolves __PLACEHOLDER__ correctly (3626 ms)

@JoshuaKGoldberg
Copy link
Member Author

+2.39%

Oof. That's not good.

split only moduleResolver

+1, I'd love to see those numbers!

@Beraliv
Copy link
Contributor

Beraliv commented Nov 22, 2022 •
edited
Loading

I've tried to split moduleResolver different ways (average for 5 runs on my Macbook Air M1):

Just one separate file

if I just move the body of moduleResolver describe, it makes it faster by ~11%

Command Mean [s] Min [s] Max [s] Relative Diff ((main - branch) / main x 100%)
yarn jest parse (no cache, main) 30.375 ± 0.978 29.255 31.468 1.00 0%
yarn jest parse (with cache, main) 29.299 ± 0.360 28.858 29.833 1.00 0%
yarn jest parse (no cache, 1 moduleResolver) 27.043 ± 0.387 26.833 27.731 1.00 -10.97%
yarn jest parse (with cache, 1 moduleResolver) 25.415 ± 0.538 24.806 25.984 1.00 -13.26%

Branch – test/parallelisation-2

N separate files for each long describe

When I split parseAndGenerateServices into these files (see below), I see improvement only by 1.75% (warn run is much faster but cold one is almost same as it was in main)

Command Mean [s] Min [s] Max [s] Relative Diff ((main - branch) / main x 100%)
yarn jest parse (no cache, main) 30.375 ± 0.978 29.255 31.468 1.00 0%
yarn jest parse (with cache, main) 29.299 ± 0.360 28.858 29.833 1.00 0%
yarn jest parse (no cache, N moduleResolver) 29.843 ± 0.904 28.884 30.883 1.00 -1.75%
yarn jest parse (cache, N moduleResolver) 25.544 ± 0.319 25.227 26.065 1.00 -12.82%

The files are the following for this approach:

  • parse.moduleResolver.default-program-error.test.ts
  • parse.moduleResolver.default-program-success.test.ts
  • parse.moduleResolver.placeholder-error.test.ts
  • parse.moduleResolver.placeholder-success.test.ts

Branch – test/parallelisation-3

I will double check on my MacBook Pro as I feel it's a little bit biased to MacBook Air

@Beraliv
Copy link
Contributor

Beraliv commented Nov 22, 2022

Another check for MacBook Pro 2.3 GHz 8-Core Intel Core i9

Command Mean [s] Min [s] Max [s] Relative Diff ((main - branch) / main x 100%)
yarn jest parse (no cache, main) 31.185 ± 0.587 30.764 32.221 1.00 0%
yarn jest parse (with cache, main) 29.330 ± 0.059 29.253 29.411 1.00 0%
yarn jest parse (no cache, 1 moduleResolver) 36.841 ± 3.762 32.399 41.993 1.00 +18.14%
yarn jest parse (with cache, 1 moduleResolver) 35.364 ± 3.907 29.685 39.842 1.00 +20.57%
yarn jest parse (no cache, N moduleResolver) 24.311 ± 1.148 23.217 25.992 1.00 -22.04%
yarn jest parse (with cache, N moduleResolver) 24.087 ± 3.323 20.569 29.384 1.00 -17.88%

So I think maybe parallelisation isn't working properly on my MacBook Air M1, here we see significant improvement for N moduleResolver split

@Beraliv
Copy link
Contributor

Beraliv commented Nov 22, 2022

When I split parseAndGenerateServices into these files (see below), I see degradation of performance from 30.375 to 31.100 (+2.39%) in average for 5 runs on my Macbook Air M1.

  1. moduleResolver
  2. projectFolderIgnoreList
  3. invalid project error messages
  4. preserveNodeMaps
  5. rest of tests

For more information, please check #6049

I have another suggestion: maybe it makes sense to split only moduleResolver as most of the time (14s out of 22s, or 63.6%, see the spoiler with my run below) was spent on it?

Output on Macbook Air M1 (main branch)

I've also run the examples from PR for MacBook Pro, and the result isn't that bad

Command Mean [s] Min [s] Max [s] Relative Diff ((main - branch) / main x 100%)
yarn jest parse (no cache, main) 31.185 ± 0.587 30.764 32.221 1.00 0%
yarn jest parse (with cache, main) 29.330 ± 0.059 29.253 29.411 1.00 0%
yarn jest parse (no cache, 5 parse) 27.834 ± 2.477 26.427 32.235 1.00 -10.74%
yarn jest parse (with cache, 5 parse) 24.981 ± 0.684 24.555 26.169 1.00 -14.83%

@JoshuaKGoldberg
Copy link
Member Author

Cool! So @Beraliv would you say the PR is ready to un-draft? I'm up for accepting these changes - you've by far shown that they're not just non-significantly-worse, but very beneficial.

@Beraliv
Copy link
Contributor

Beraliv commented Nov 25, 2022

Cool! So @Beraliv would you say the PR is ready to un-draft? I'm up for accepting these changes - you've by far shown that they're not just non-significantly-worse, but very beneficial.

I've seen faster runs with N moduleResolver split so I think I will close my current PR and will open another one where I split moduleResolver only

I will make it ready later today

@Beraliv Beraliv mentioned this issue Nov 25, 2022
Merged
3 tasks
@Beraliv
Copy link
Contributor

Beraliv commented Nov 25, 2022 •
edited
Loading

I've created one @JoshuaKGoldberg, it's here – #6092

@Beraliv
Copy link
Contributor

Beraliv commented Dec 28, 2022

Apologies for the late progress, I'm back from vacation today, will quickly update the PR

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue good first issue Good for newcomers repo maintenance things to do with maintenance of the repo, and not with code/docs
Projects
None yet
Milestone
No milestone
3 participants
@Beraliv @JoshuaKGoldberg @bradzacher