Skip to content

Conversation

nossbigg
Copy link
Contributor

@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
Contributor 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
Contributor 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.

@nossbigg
Copy link
Contributor Author

@gnapse any chance to look at it? 👋

@gnapse
Copy link
Member

gnapse commented Sep 30, 2025

It was less than 100% coverage. There were a couple of cases not covered by the tests. See e0d4659.

@gnapse gnapse merged commit 95f870a into testing-library:main Sep 30, 2025
7 checks passed
Copy link

🎉 This PR is included in version 6.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nossbigg
Copy link
Contributor Author

@gnapse thanks for adding the test and landing the PR! 🙇

colinaaa added a commit to lynx-family/lynx-stack that referenced this pull request Oct 1, 2025
`@testing-library/jest-dom` has published a new version
[v6.9.0](https://github.com/testing-library/jest-dom/releases/tag/v6.9.0),
which would reference the `Node` from global. See:
testing-library/jest-dom#702

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- New Features
  - Added a test script to run Vitest in the testing environment.

- Bug Fixes
- Fixed a “Node is not defined” error in Vitest setup, improving test
stability.

- Chores
- Updated @testing-library/jest-dom to v6.9.0 across packages and
templates.
- Added patch dependencies: @lynx-js/react and
@lynx-js/testing-environment.

- Tests
- Improved test environment configuration for compatibility with latest
testing libraries.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

## Checklist

<!--- Check and mark with an "x" -->

- [ ] Tests updated (or not required).
- [ ] Documentation updated (or not required).
- [x] Changeset added, and when a BREAKING CHANGE occurs, it needs to be
clearly marked (or not required).
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.

2 participants