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

fix(getQueryNodeFrom): move Identifier type checks to getInnerNodeFrom #255

Merged

Conversation

ricardozv28
Copy link
Contributor

@ricardozv28 ricardozv28 commented Jan 21, 2022

What:
Move logic for checking if expression type is Identifier to getInnerNodeFrom method

Why:
As reported on issue #203, features like .toBeDisabled() are not getting recommend as fixes for code like the following:

const button = getByRole('button', { name: 'My Button' });
expect(button.disabled).toBe(true);

How:
After a little bit of digging, I noticed that when using getByRole method the type of the expression that was getting passed to getInnerNodeFrom was a MemberExpression, which was being handled by in the switch statement, expression.object was getting passed now to getInnerNodeFrom which was the type of 'Identifier' but since that case is not getting handled in the method, the returned expression didn't have a property .callee which was making getQueryNodeFrom to return isDTLQuery as false

Checklist:

  • Documentation N/A
  • Tests N/A
  • Ready to be merged

Fix #203

@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #255 (5ddce16) into main (48fce31) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #255   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          581       581           
  Branches       167       165    -2     
=========================================
  Hits           581       581           
Impacted Files Coverage Δ
src/assignment-ast.js 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@MichaelDeBoey MichaelDeBoey changed the title fix(getQueryNodeFrom): move checks for Identifier type to getInnerNodeFrom fix(getQueryNodeFrom): move Identifier type checks to getInnerNodeFrom May 26, 2022
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a few test cases please?

@ricardozv28
Copy link
Contributor Author

Can you add a few test cases please?

trying to pick this up again in my free time, do you have some existing unit test examples that I can take a look at for reference??

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 14, 2023

@ricardozv28 tests for rules live here - you don't need to add tests to every rule, but we should have a couple if possible (including one here for toBeDisabled)

@MichaelDeBoey MichaelDeBoey merged commit 96c364a into testing-library:main Jun 4, 2023
63 checks passed
@G-Rath G-Rath mentioned this pull request Jun 4, 2023
3 tasks
@github-actions
Copy link

github-actions bot commented Jun 4, 2023

🎉 This PR is included in version 5.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ricardozv28 ricardozv28 deleted the check_identifier_type branch January 22, 2024 16:02
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 this pull request may close these issues.

Latest version not reporting/suggesting changes on disabled/value attributes
3 participants