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

Allow saving "within" to variable for rule "Suggest using screen while querying (testing-library/prefer-screen-queries)" #372

Open
florianmatz opened this issue May 6, 2021 · 7 comments
Labels
bug Something isn't working

Comments

@florianmatz
Copy link

According to the rule using

within(screen.getByTestId('section')).getByText('foo');

is allowed.

However, saving the result to a variable

const context = within(screen.getByTestId('section')).getByText('foo');

to re-use it, results in the warning.

@Belco90
Copy link
Member

Belco90 commented May 6, 2021

Hi @florianmatz!

Could you confirm what version of the plugin are you using? Thanks!

@Belco90 Belco90 added the awaiting response Waiting for a reply from the OP label May 6, 2021
@florianmatz
Copy link
Author

florianmatz commented May 6, 2021

Hi, thanks for the fast reply:

Using the following packages:

"devDependencies": {
    "@testing-library/dom": "^7.30.4",
    "@types/jest": "^26.0.23",
    "@types/node": "^15.0.2",
    "@types/react": "^17.0.5",
    "@types/react-dom": "^17.0.3",
    "@typescript-eslint/eslint-plugin": "^4.22.1", 
    "@typescript-eslint/parser": "^4.22.1",
    "babel-eslint": "^10.1.0",
    "eslint": "^7.25.0",
    "eslint-config-airbnb": "^18.2.1",
    "eslint-config-airbnb-typescript": "^12.3.1",
    "eslint-config-prettier": "^8.3.0",
    "eslint-config-react-app": "^6.0.0",
    "eslint-plugin-flowtype": "^5.7.2",
    "eslint-plugin-import": "^2.22.1",
    "eslint-plugin-jest-dom": "^3.8.1",
    "eslint-plugin-jsx-a11y": "^6.4.1",
    "eslint-plugin-prettier": "^3.4.0",
    "eslint-plugin-react": "^7.23.2",
    "eslint-plugin-react-hooks": "^4.2.0",
    "eslint-plugin-testing-library": "^4.2.0",
    "typescript": "^4.2.4"
  },

Config:

  "extends": [
      "react-app",
      "airbnb-typescript",
      "airbnb/hooks",
      "plugin:@typescript-eslint/recommended",
      "plugin:prettier/recommended",
      "plugin:jest-dom/recommended",
      "plugin:testing-library/react"
    ],
    "ignorePatterns": "src/**/*.json",
    "plugins": [
      "react",
      "jest-dom",
      "testing-library",
      "@typescript-eslint"
    ],

@Belco90 Belco90 added bug Something isn't working and removed awaiting response Waiting for a reply from the OP labels May 6, 2021
@Belco90
Copy link
Member

Belco90 commented May 6, 2021

This is a legit bug, thanks for reporting! I think I have an idea where the issue is located. I'll try to handle this one soon after #359 and #368.

In case someone else takes care of this ticket: The problem is that prefer-screen-queries is too tight to a very specific AST shape so it doesn't handle other cases like this one properly. The rule must be refactored to navigate and visit nodes using our node-utils like getDeepestIdentifierNode, getPropertyIdentifierNodeetc as in the rest of the rules.

@Belco90 Belco90 self-assigned this May 9, 2021
@Belco90 Belco90 added the awaiting response Waiting for a reply from the OP label May 18, 2021
@Belco90
Copy link
Member

Belco90 commented May 18, 2021

Hi again @florianmatz! I've tried to reproduce the problem you described, but it seems to be working fine. Could you check if the example you mentioned is the correct one to reproduce the issue, please?

@florianmatz
Copy link
Author

Hi, yeah it still is. Kind of...

It does not warn, when I save the context directly in a test:

const context = within(headline);
context.someQuery...

But, when I have for instance a helper function that returns a context

const getDialog = () => {
...
const dialogContext = within(dialog);

return {dialog, dialogContext);
}

const {dialog, dialogContext} = getDialog();

dialogContext.someQuery => WARNING

So, there is some improvement but in some conditions the problem still exists. Sorry for the late response.

@Belco90
Copy link
Member

Belco90 commented May 26, 2021

Thanks for the new example. I don't think this rule is ready nested variables like this, so it would need some improvements. I can't handle this at the moment, but leaving it as a bug in case someone else can take care of it.

@Belco90 Belco90 removed the awaiting response Waiting for a reply from the OP label May 26, 2021
@Belco90 Belco90 removed their assignment Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants
@Belco90 @florianmatz and others