Skip to content

Conversation

nossbigg
Copy link

@nossbigg nossbigg commented Sep 13, 2025

What:

  • Add .toAppearBefore()/.toAppearAfter() matchers

Why:

How:

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged

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.

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.

Copy link
Member

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?

Copy link
Author

@nossbigg nossbigg Sep 16, 2025

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 span
  • B is nested span

Then

  • expect(a).toAppearBefore(b), it will return Received: Unknown document position (20)
  • expect(b).toAppearBefore(a), it will return Received: 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 ✅

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?

Copy link
Member

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.

Copy link
Author

@nossbigg nossbigg Sep 16, 2025

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?

Copy link
Member

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.

@gnapse
Copy link
Member

gnapse commented Sep 16, 2025

@all-contributors please add @nossbigg for code, test

Copy link
Contributor

@gnapse

I've put up a pull request to add @nossbigg! 🎉

@gnapse
Copy link
Member

gnapse commented Sep 16, 2025

CI is failing all around now and I have no time to look into it. Will look into it later this week.

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

Successfully merging this pull request may close these issues.

2 participants