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

False positive for await-async-utils #732

Closed
iantocristian opened this issue Feb 10, 2023 · 7 comments · Fixed by #733
Closed

False positive for await-async-utils #732

iantocristian opened this issue Feb 10, 2023 · 7 comments · Fixed by #733
Labels

Comments

@iantocristian
Copy link

iantocristian commented Feb 10, 2023

Have you read the Troubleshooting section?

Yes

Plugin version

v5.10.1

ESLint version

v8.33.0

Node.js version

v18.12.1

package manager and version

npm 9.4.2

Operating system

macOS Ventura 13.2

Bug description

This appears to be a regression introduced with the latest patch release, v5.10.1. Not present in v5.10.0.

False positive rule await-async-utils triggered by inline async function definition on other variables and/or definitions.

Promise returned from x wrapper over async util must be handled but x is not a function.

Steps to reproduce

This code triggers a false positive on expectedValue:

import React from 'react';
import { render, act } from '@testing-library/react';

function wait(ms) {
  return new Promise((resolve) => {
    setTimeout(resolve, ms);
  });
}

const waitWithAct = async (timeout) => {
  await act(async () => await wait(timeout));
};

describe('Component', () => {
  const mock = jest.fn();

  it('test', async () => {
    let Component = () => {
      mock(1);
      return <div />;
    };
    render(<Component />);

    await waitWithAct(500);

    const expectedValue = 1;
    expect(mock).toHaveBeenCalledWith(expectedValue);
  });
});

Rewriting the inline async function fixes the false positive:

import React from 'react';
import { render, act } from '@testing-library/react';

function wait(ms) {
  return new Promise((resolve) => {
    setTimeout(resolve, ms);
  });
}

const waitWithAct = async (timeout) => {
  await act(async () => {
    await wait(timeout);
  });
};

describe('Component', () => {
  const mock = jest.fn();

  it('test', async () => {
    let Component = () => {
      mock(1);
      return <div />;
    };
    render(<Component />);

    await waitWithAct(500);

    const expectedValue = 1;
    expect(mock).toHaveBeenCalledWith(expectedValue);
  });
});

Another case of false positive, this results in false positives reported for mock and jest.fn:

import React from 'react';
import { render, act } from '@testing-library/react';

function wait(ms) {
  return new Promise((resolve) => {
    setTimeout(resolve, ms);
  });
}

const waitWithAct = async (timeout) => {
  const fn = async () => await wait(timeout);
  await act(fn);
};

describe('Component', () => {
  const mock = jest.fn();

  it('test', async () => {
    let Component = () => {
      mock(1);
      return <div />;
    };
    render(<Component />);

    await waitWithAct(500);

    const expectedValue = 1;
    expect(mock).toHaveBeenCalledWith(expectedValue);
  });
});

Error output/screenshots

No response

ESLint configuration

module.exports = {
  env: {
    es6: true,
  },
  parserOptions: {
    ecmaVersion: 2021,
    ecmaFeatures: {
      jsx: true,
    },
    sourceType: 'module',
  },
  globals: {
    process: true,
    module: true,
  },
  extends: ['plugin:prettier/recommended'],
  plugins: ['prettier', 'eslint-plugin-no-only-tests', 'deprecate', 'jest', 'testing-library', 'cypress'],
  overrides: [
    {
      files: ['*.test.jsx', '*.test.js', 'jest/setup-tests.js'],
      extends: ['plugin:jest/recommended', 'plugin:testing-library/react'],
    },
  ],
};

Rule(s) affected

testing-library/await-async-utils

Do you want to submit a pull request to fix this bug?

No

@iantocristian iantocristian added bug Something isn't working triage Pending to be triaged by a maintainer labels Feb 10, 2023
@Belco90 Belco90 removed the triage Pending to be triaged by a maintainer label Feb 11, 2023
@Belco90
Copy link
Member

Belco90 commented Feb 11, 2023

Thanks for reporting @iantocristian! This is a regression introduced in the last version indeed.

@Belco90
Copy link
Member

Belco90 commented Feb 11, 2023

Pinging @patriscus in case you can check this issue.

@patriscus
Copy link
Contributor

I'm sorry to see that I've caused some issues 👀 I had a quick look:

The first example seems to be related to the isAssigningKnownAsyncFunctionWrapper I introduced in my latest commit. Using getDeepestIdentifierNode(node.init)?.name ?? '' actually returns true for expectedValue, since we fallback to the default (''). Interestingly, '' is actually a part of the functionWrapperNames since the getFunctionName(innerFunction) call within the detectAsyncUtilWrapper function pushes an empty string into the array.

I very quickly added a safeguard to not push empty strings into the array, and the tests seem to pass (need to verify it a bit more thoroughly).

The second example is more complex in my eyes. This code fails even before my commit - my theory is that since we push wrapper names, we don't really check scopes. So in the example provided:

const waitWithAct = async (timeout) => {
  const fn = async () => await wait(timeout);
  await act(fn);
};

We will push fn into the functionWrapperNames. Later, by using jest.fn(), it's mistakenly flagged as a wrapper around a util because fn matches. So I guess we need to somehow get the "true" variables/references, instead of "just" comparing names.

I could be wrong, though.

I will try to provide a PR for the first issue by tomorrow's EOD.

About the second example, I don't know if it makes sense to do it in this issue. It seems to me that this is a separate topic; hence I wanted to ask if it should be extracted into a separate issue. I will for sure not be able to solve it within the next 2 days, and I'm not available for a couple of days (vacation). I can, however, pick it up after I'm back (would just give a quick ping to let you know).

Please let me know what you think @Belco90.

@iantocristian
Copy link
Author

Thanks for addressing this issue!

@Belco90
Copy link
Member

Belco90 commented Feb 15, 2023

@patriscus Better to address the issues in separate PRs!

@github-actions
Copy link

🎉 This issue has been resolved in version 5.10.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Aug 5, 2023

🎉 This issue has been resolved in version 6.0.0-alpha.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

3 participants