-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: Specify baseElement without container #394
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
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.
Thanks for this! I think the change is necessary. The test could use a little more work, please.
src/__tests__/render.js
Outdated
@@ -113,3 +113,44 @@ test('renders options.wrapper around node', () => { | |||
</div> | |||
`) | |||
}) | |||
|
|||
describe('baseElement', () => { |
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 really prefer to not use any nesting. This is all for just one test, please instead move the contents of beforeAll and afterAll to the inside of the test and use the test
global rather than the describe
and it
globals.
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.
This breaks two other test that assume an empty body. The nesting is for isolation. What is the problem with that? The alternative is to create a new test which is worse IMO.
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 don't like the direction that nesting puts us in. It's a slippery slope IMO. Maybe I'm just too scarred by past testbases. I'm much more willing to create a new test file than nesting tests.
Another thing we could do is just take the beforeAll
and afterAll
and put them inside the test. That would avoid the problems altogether. Could you do that please?
src/__tests__/render.js
Outdated
}) | ||
|
||
it('can take a custom element for isolation', () => { | ||
function DescribedButton({title: description, ...buttonProps}) { |
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.
Is it necessary that this component be so complex? I prefer to make the component simpler/contrived to get the point across. I believe that this test would pass with or without the baseElement
... Can we have a test that actually requires the baseElement
?
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'll give it a try to make this more meaningful. It does, however, fail without the fix applied or without the baseElement passed.
Hope that makes more sense. |
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.
LGTM. Thanks!
🎉 This PR is included in version 8.0.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
## The devDependency [@testing-library/react](https://github.com/testing-library/react-testing-library) was updated from `8.0.1` to `8.0.2`. This version is **not covered** by your **current version range**. If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update. --- <details> <summary>Release Notes for v8.0.2</summary> <h2><a href="https://urls.greenkeeper.io/testing-library/react-testing-library/compare/v8.0.1...v8.0.2">8.0.2</a> (2019-06-24)</h2> <h3>Bug Fixes</h3> <ul> <li>Specify baseElement without container (<a href="https://urls.greenkeeper.io/testing-library/react-testing-library/issues/394" data-hovercard-type="pull_request" data-hovercard-url="/testing-library/react-testing-library/pull/394/hovercard">#394</a>) (<a href="https://urls.greenkeeper.io/testing-library/react-testing-library/commit/8e99f3c">8e99f3c</a>)</li> </ul> </details> <details> <summary>Commits</summary> <p>The new version differs by 2 commits.</p> <ul> <li><a href="https://urls.greenkeeper.io/testing-library/react-testing-library/commit/8e99f3cf99b76afbf2b20993745b802def3bb6c9"><code>8e99f3c</code></a> <code>fix: Specify baseElement without container (#394)</code></li> <li><a href="https://urls.greenkeeper.io/testing-library/react-testing-library/commit/3076b119172b8b7188603509bfe576b7d2914de3"><code>3076b11</code></a> <code>docs: add pascalduez as a contributor (#383)</code></li> </ul> <p>See the <a href="https://urls.greenkeeper.io/testing-library/react-testing-library/compare/76d112cff16823be1874f1baa265dad5753c127c...8e99f3cf99b76afbf2b20993745b802def3bb6c9">full diff</a></p> </details> <details> <summary>FAQ and help</summary> There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new). </details> --- Your [Greenkeeper](https://greenkeeper.io) bot 🌴
What:
Fix
baseElement
being overridden if nocontainer
was providedWhy:
I'm having a hard time grasping the difference between
baseElement
andcontainer
in the current implementation:-- https://testing-library.com/docs/react-testing-library/api#baseelement-1
Considering the default values I imagined it as
The
base-element
acts as a long living container that will hold every rendered container but can be isolated from the rest of the document.What I'm basically trying to achieve is that the queries don't look inside document.body but rather only in the DOM tree rendered by the component. It's not so much about portals but other react trees living in the same document that should be ignored.
How:
Checklist:
docs site Note: Should I rather update the documentation?
[ ] Typescript definitions updated