-
Notifications
You must be signed in to change notification settings - Fork 411
feat: Add .toAppearBefore/.toAppearAfter matcher #702
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Thanks!
There's one main comment below in this review that I'd like to see addressed before we merge it. The one about what happens if the elements are parent/ancestor to each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the expected behavior if the two elements are parent/ancestor? That is:
<div>
<div data-testid='div-a'>
<span data-testid='text-a'>
Text A
<span data-testid='text-b'>Text B</span>
</span>
</div>
</div>
Whatever is it (I hope it is a logical behavior), can we document that behavior? And also add it to the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gnapse given:
A
is parent spanB
is nested span
Then
expect(a).toAppearBefore(b)
, it will returnReceived: Unknown document position (20)
expect(b).toAppearBefore(a)
, it will returnReceived: Unknown document position (10)
Note: Unknown document position
is a default fill when we can't find a corresponding string representation (ref) for the returned compareDocumentPosition()
value.
I've covered the parent/child scenarios here ✅
jest-dom/src/__tests__/to-appear-before.js
Lines 31 to 41 in efba541
it('errors out when first element is parent of second element', () => { | |
expect(() => expect(divA).toAppearBefore(textA)).toThrowError( | |
/Received: Unknown document position \(20\)/i, | |
) | |
}) | |
it('errors out when first element is child of second element', () => { | |
expect(() => expect(textA).toAppearBefore(divA)).toThrowError( | |
/Received: Unknown document position \(10\)/i, | |
) | |
}) |
I think there's three ways that we can go with this:
- Treat parent as "before": this will avail the
.toAppearBefore()
matcher to use cases where devs are trying to test against parent/child relationships, but otherwise makes the matcher more lax. - Do not treat parent as "before": This constrains the
.toAppearBefore()
matcher to strictly only sibling use cases, which is more exacting, but leaves devs out in the cold when it comes to testing parent/child relationships (ie. they'll have to write their own test helper for it) - Add additional param to allow user to set if parent should be treated as "before": This gives users the flexibility to opt-in (or opt-out, depending on what makes for best defaults) the aforementioned behavior in Point 1.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards thinking that the behavior I'd expect is that neither element is before or after the other. So both assertions would fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok - the current implementation does error out on parent/child relationships. Shall I outline this behavior in the docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it errors out in both cases, that's ok.
@all-contributors please add @nossbigg for code, test |
I've put up a pull request to add @nossbigg! 🎉 |
CI is failing all around now and I have no time to look into it. Will look into it later this week. |
What:
.toAppearBefore()
/.toAppearAfter()
matchersWhy:
How:
compareDocumentPosition()
Checklist: