Skip to content

feat(toHaveAttribute): support asymmetric matchers for value #93

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

Conversation

revathskumar
Copy link
Contributor

@revathskumar revathskumar commented Apr 15, 2019

Fixes #91

Sample Error message
Screenshot from 2019-04-14 19-47-15

What:

toHaveAttribute value will accept asymmetric matchers

Why:

asymmetric matchers are handy when we want to do partial matching

How:

Using the method equals in expect/JasmineUtils

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged
  • Added myself to contributors table

@revathskumar
Copy link
Contributor Author

@gnapse Working on updating the docs.

@revathskumar revathskumar force-pushed the asymmetricMatch-for-toHaveAttribute branch from 632fc3d to ff83b2b Compare April 15, 2019 13:54
@revathskumar
Copy link
Contributor Author

Updated the documentation

Copy link
Member

@gnapse gnapse left a comment

Choose a reason for hiding this comment

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

This is looking good! Just a comment to fix a small typo, and let's take a bit more time to check the types situation.

@revathskumar
Copy link
Contributor Author

@gnapse ping

@gnapse
Copy link
Member

gnapse commented May 6, 2019

@revathskumar I'm ready to merge. Can you fix the conflict in the README? You should be able to rebase this on top of master, and when faced with the conflict locally, run npm run precommit, which should rewrite this part of the README. Then git add and git rebase --continue. I'd do it but I'd need to add your repo as a new remote in git and download your branch. I can try later, but if can get to it first, that'd be great.

And thanks again for your contribution!

@revathskumar revathskumar force-pushed the asymmetricMatch-for-toHaveAttribute branch from 62e2084 to deb867b Compare May 6, 2019 20:25
@revathskumar
Copy link
Contributor Author

@gnapse I have rebased and pushed.
But seems like travis is not triggered.

@gnapse gnapse merged commit f62628e into testing-library:master May 6, 2019
@gnapse
Copy link
Member

gnapse commented May 6, 2019

🎉 This PR is included in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gnapse gnapse added the released label May 6, 2019
@bopfer
Copy link
Contributor

bopfer commented May 6, 2019

I think the typings are breaking things. I get this after upgrading:

./node_modules/jest-dom/extend-expect.d.ts:11:5 - error TS2717: Subsequent property declarations must have the same type.  Property 'not' must be of type 'InverseAsymmetricMatchers', but here has type 'InverseStringAsymmetricMatchers'.

11     not: InverseStringAsymmetricMatchers
       ~~~

./node_modules/jest-dom/extend-expect.d.ts:16:7 - error TS2713: Cannot access 'Expect.stringContaining' because 'Expect' is a type, but not a namespace. Did you mean to retrieve the type of the property 'stringContaining' in 'Expect' with 'Expect["stringContaining"]'?

16     | Expect.stringContaining
         ~~~~~~~~~~~~~~~~~~~~~~~

./node_modules/jest-dom/extend-expect.d.ts:17:7 - error TS2713: Cannot access 'Expect.stringMatching' because 'Expect' is a type, but not a namespace. Did you mean to retrieve the type of the property 'stringMatching' in 'Expect' with 'Expect["stringMatching"]'?

17     | Expect.stringMatching
         ~~~~~~~~~~~~~~~~~~~~~

./node_modules/jest-dom/extend-expect.d.ts:18:7 - error TS2713: Cannot access 'Expect.any' because 'Expect' is a type, but not a namespace. Did you mean to retrieve the type of the property 'any' in 'Expect' with 'Expect["any"]'?

18     | Expect.any
         ~~~~~~~~~~

./node_modules/jest-dom/extend-expect.d.ts:19:7 - error TS2713: Cannot access 'Expect.anything' because 'Expect' is a type, but not a namespace. Did you mean to retrieve the type of the property 'anything' in 'Expect' with 'Expect["anything"]'?

19     | Expect.anything
         ~~~~~~~~~~~~~~~

./node_modules/jest-dom/extend-expect.d.ts:20:7 - error TS2713: Cannot access 'Expect.not' because 'Expect' is a type, but not a namespace. Did you mean to retrieve the type of the property 'not' in 'Expect' with 'Expect["not"]'?

20     | Expect.not
         ~~~~~~~~~~

@mdgozza
Copy link
Contributor

mdgozza commented May 6, 2019

Fix for type regression: #101

@@ -1,4 +1,5 @@
import {matcherHint, stringify, printExpected} from 'jest-matcher-utils'
import {equals} from 'expect/build/jasmineUtils'
Copy link
Contributor

Choose a reason for hiding this comment

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

equals is injected into custom matchers as this.equals: https://jestjs.io/docs/en/expect#thisequalsa-b, it'd be great if you could use that instead

We provide no guarantees that 'expect/build/jasmineUtils' is an import that will not break at any time

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the tip @SimenB!

@revathskumar can you take that since you originally authored this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gnapse Sure. will look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gnapse @SimenB please review #103

@revathskumar revathskumar deleted the asymmetricMatch-for-toHaveAttribute branch May 8, 2019 15:15
revathskumar added a commit to revathskumar/jest-dom that referenced this pull request May 8, 2019
As per the [comment](testing-library#93 (comment)) by @SimenB

This will remove the direct import of jasmineUtils

`import {equals} from 'expect/build/jasmineUtils'`

and instead use `this.equals`.

Also removes the added dependency of `expect`.
gnapse pushed a commit that referenced this pull request May 8, 2019
As per the [comment](#93 (comment)) by @SimenB

This will remove the direct import of jasmineUtils

`import {equals} from 'expect/build/jasmineUtils'`

and instead use `this.equals`.

Also removes the added dependency of `expect`.
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.

support for asymmetricMatch as the value for toHaveAttribute matcher
5 participants