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

Bug: An error occurs when invoking ESLint#lintFiles() in Jest #18407

Closed
1 task done
StewEucen opened this issue May 1, 2024 · 8 comments · Fixed by #18416
Closed
1 task done

Bug: An error occurs when invoking ESLint#lintFiles() in Jest #18407

StewEucen opened this issue May 1, 2024 · 8 comments · Fixed by #18416
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features repro:yes

Comments

@StewEucen
Copy link

StewEucen commented May 1, 2024

Environment

Node version: 20.12.2
npm version: 10.5.0
Local ESLint version: 9.1.1
Global ESLint version: 9.1.1
Operating System: macOS

What parser are you using?

Default (Espree)

What did you do?

Configuration
  • eslint.config.js

    'use strict'
    
    module.exports = [
      {
        languageOptions: {
          parserOptions: {
            ecmaVersion: 'latest',
          },
        },
        ignores: [
          '**/node_modules/**',
        ],
        rules: {
          indent: ['error', 2],
          quotes: ['error', 'single'],
          semi: ['error', 'never'],
        },
      },
    ]
  • Actual code

    'use strict'
    
    const {
      ESLint,
    } = require('eslint')
    
    describe('ESLint Bug', () => {
      test('should not throw an error', async () => {
        const eslint = new ESLint()
        const filePaths = [
          'tests/resources',
        ]
        const expectedLength = 3
    
        const results = await eslint.lintFiles(filePaths)
    
        expect(results)
          .toHaveLength(expectedLength)
      })
    })
  • Test Resources

    • tests/resources/indent.js

      'use strict'
      
      module.exports = function doubleValue (
        value,
        ignore
      ) {
        if (ignore) {
          return value
          }
      
        return value + value
      }
    • tests/resources/quotes.js

      'use strict'
      
      module.exports = "quotes"
    • tests/resources/semi.js

      'use strict'
      
      global.semi = 111;

What did you expect to happen?

  • I expect that Jest shows the log below

     PASS  tests/__tests__/eslint-bug.js
      ESLint Bug
        ✓ should not throw an error (121 ms)
    
    Test Suites: 1 passed, 1 total
    Tests:       1 passed, 1 total
    Snapshots:   0 total
    Time:        1.221 s, estimated 2 s
    Ran all test suites.

What actually happened?

  • An error Result is not a promise. is thrown.

     FAIL  tests/__tests__/eslint-bug.js
      ESLint Bug
        ✕ should not throw an error (105 ms)
    
      ● ESLint Bug › should not throw an error
    
        Result is not a promise.
    
          13 |     const expectedLength = 3
          14 |
        > 15 |     const results = await eslint.lintFiles(filePaths)
             |                     ^
          16 |
          17 |     expect(results)
          18 |       .toHaveLength(expectedLength)
    
          at Retrier.retry (node_modules/@humanwhocodes/retry/dist/retrier.cjs:200:35)
              at Array.map (<anonymous>)
          at Object.<anonymous> (tests/__tests__/eslint-bug.js:15:21)
    
    Test Suites: 1 failed, 1 total
    Tests:       1 failed, 1 total
    Snapshots:   0 total
    Time:        1.195 s
    Ran all test suites.

Link to Minimal Reproducible Example

https://github.com/StewEucen/eslint-bug-with-jest

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

@StewEucen StewEucen added bug ESLint is working incorrectly repro:needed labels May 1, 2024
@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features repro:yes and removed repro:needed labels May 1, 2024
@mdjermanovic
Copy link
Member

I can reproduce this locally. Thanks for the detailed analysis of the cause!

https://github.com/StewEucen/eslint-bug-with-jest?tab=readme-ov-file#cause

  • Retrier#retry() uses Promise declared in lib.es2015.iterable.d.ts
  • fs.readFile() uses Promise declared in lib.es5.d.ts

So it seems that in Node.js different operations may use different Promise constructors?

@nzakas perhaps @humanwhocodes/retry could check if the result is a thenable instead of checking result instanceof Promise?

@StewEucen
Copy link
Author

StewEucen commented May 1, 2024

@mdjermanovic

  • Thank you for your confirmation :)

So it seems that in Node.js different operations may use different Promise constructors?

  • I think that since fs is old package, it uses polyfill Promise class.

  • I have already found two ways to fix now.

    1. Change argument of #retry() function to async in ESLint repository.

      -                return retrier.retry(() => fs.readFile(...)
      +                return retrier.retry(async () => fs.readFile(...)
    2. Change the logic to confirm Promise in #retry() of @humanwhocodes/retry repository.

      -        if (!(result instanceof Promise)) {
      +        if (
      +          !(result instanceof Promise)
      +          && (!result || typeof result.then !== 'function')
      +        ) {
  • I think that 2. is a fixing ad-hoc, thus we can select 1.

StewEucen added a commit to StewEucen/eslint that referenced this issue May 1, 2024
@fasttime
Copy link
Member

fasttime commented May 1, 2024

The Promise object returned by fs.readFile is from a different realm:

image

This only occurs when the Node.js process is launched with the --experimental-vm-modules flag. It may have something to do with Jest's experimental ESM support, which is enabled by the flag.

@StewEucen
Copy link
Author

StewEucen commented May 2, 2024

@fasttime

  • Thank you for your information

  • I used --experimental-vm-modules on run Jest, it was due to the following reasons.
  1. I had used ESLint#lintFiles() in Jest with ESLint@8.x.x. As we know, Jest works as CommonJS. But It did not require to use --experimental-vm-modules flag.

  2. I changed the code to fit Flat Config with 9.1.1, but it was not work. Because new code of ESLint#lintFiles() uses dynamic import.

    https://github.com/eslint/eslint/blob/v9.1.1/lib/eslint/eslint.js#L317

    async function loadFlatConfigFile(filePath) {
        ...
    
        const config = (await import(fileURL)).default;
    
        ...
    }

    https://github.com/eslint/eslint/blob/v9.1.1/lib/eslint/eslint.js#L369

    async function calculateConfigArray(eslint, { ... }) {
        ...
    
        if (configFilePath) {
            const fileConfig = await loadFlatConfigFile(configFilePath);
    
            ...
        }
    
        ...
    }

    https://github.com/eslint/eslint/blob/v9.1.1/lib/eslint/eslint.js#L815

    class ESLint {
        ...
    
        async lintFiles(patterns) {
            ...
    
            const configs = await calculateConfigArray(this, eslintOptions);
    
            ...
        }
    
        ...
    }
  3. To solve the problem, I added a --experimental-vm-modules flag for Jest script.

    https://github.com/StewEucen/eslint-bug-with-jest/blob/main/package.json#L7

    {
      "scripts": {
        "test": "export NODE_OPTIONS=\"$NODE_OPTIONS --experimental-vm-modules\"; jest --forceExit"
      },
    }
  4. And then, I got the behavior that I reported in this issue.

@nzakas
Copy link
Member

nzakas commented May 2, 2024

I can update retry. Never occurred to me that there might be a promise from another realm involved.

@nzakas
Copy link
Member

nzakas commented May 3, 2024

A new version of @humanwhocodes/retry was released as a patch version that should fix this.

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label May 4, 2024
@mdjermanovic
Copy link
Member

Here's a PR to update the dependency: #18416

@StewEucen
Copy link
Author

@nzakas @mdjermanovic @fasttime

  • Thank you for your work related to this issue.
  • I am going to use ESLint patched version for my repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features repro:yes
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants