Skip to content

The returned container is one level too high compared to other frameworks #313

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.

Sign up for GitHub

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

Jump to bottom
Closed
mcous opened this issue Feb 3, 2024 · 2 comments
Closed

The returned container is one level too high compared to other frameworks #313

mcous opened this issue Feb 3, 2024 · 2 comments
Labels
released on @next

Comments

@mcous
Copy link
Collaborator

mcous commented Feb 3, 2024 •
edited
Loading

Overview

The render function provides two options for changing how a component gets rendered into the DOM: componentOptions.target and renderOptions.container. It returns a result.container element.

const { container } = render(Component, componentOptions, renderOptions)
  • componentOptions.target is the target element the component is rendered into
    • Defaults to an empty <div> that we create
  • renderOptions.container is the element that target is inserted into
    • Defaults to document.body
  • result.container returns renderOptions.container

These option names and their behaviors are different in confusing ways from other testing-library frameworks that make svelte-testing-library a little more difficult to use than its siblings. The primary issue is that by default we return document.body rather than the created div for result.container.

Expectations

Value Svelte React / Preact / Vue
Document root option container baseElement
Mount point option target container
Returned container Document root Mount point 
const { container, component } = render(Comp, { foo: 'bar' })
<body>                 <!-- container (baseElement) -->
  <div>                <!-- target (container) -->
    <Comp foo="bar" /> <!-- component -->
  </div>
</body>

The main issue I have is result.container is one level higher than I expect. The most common way I notice this is that I expect container.firstChild to be my component's first node. Instead, it is container.firstElement.firstElement.

This expectation comes from the react-testing-library docs:

container

The containing DOM node of your rendered React Element (rendered using ReactDOM.render). It's a div. This is a regular DOM node, so you can call container.querySelector etc. to inspect the children.

Tip: To get the root element of your rendered element, use container.firstChild.

Proposed changes

Since there are definitely breaking changes on the horizon due to Svelte moving on from v3, I think there's an opportunity here to align the render API with all the other libraries.

I think a small but effective change set would be:

  • Rename the existing container option to baseElement
  • Return baseElement (root) as result.baseElement
  • Return target (the wrapping <div>) as result.container
  • If target is provided, use it as the baseElement and do not create/insert any other elements
    • This is the same behavior as (react|preact|vue)-testing-library

I think there are a few other changes we could consider:

  • Rename the existing target option to container
    • Pro: this would align better with the name of the return value
    • Con: would misalign us with the name of the Svelte option
  • Collapse componentOptions and renderOptions into a single options object
    • Pro: aligns a little better with vue-testing-library
    • Con: runs the risk of conflicts between Svelte options and testing-library options

Relates to #312
@mcous mcous mentioned this issue Feb 3, 2024
Closed
@mcous mcous mentioned this issue Feb 13, 2024
Merged
@yanick
Copy link
Collaborator

yanick commented Feb 14, 2024

First, kudos for the detailed explanations. Even with that, I had to re-read it many times before my head stopped swimming with the overloaded meanings of target and container. :-)

I think a small but effective change set would be:

Rename the existing container option to baseElement
Return baseElement (root) as result.baseElement
Return target (the wrapping

) as result.container
If target is provided, use it as the baseElement and do not create/insert any other elements
This is the same behavior as (react|preact|vue)-testing-library

That all make sense to me. It aligns the terminologies as best as possible.

I think there are a few other changes we could consider:

Rename the existing target option to container
Pro: this would align better with the name of the return value
Con: would misalign us with the name of the Svelte option

I think we're better to stick with target. In my opinion, Svelte terminology compatibility is the number 1 priority, and alignment with other testing-libraries being number 2. Moreover that with this render right now we can say that the first argument is what we throw verbatim (ish) at the Svelte Component class. If we begin to add exceptions, it'll muddy more the waters than it'll help things (or so I think).

Collapse componentOptions and renderOptions into a single options object
Pro: aligns a little better with vue-testing-library
Con: runs the risk of conflicts between Svelte options and testing-library options

Yeeeah, it's one of those that sounds good because it's slightly less typing, but what was "first argument is what svelte needs -- you know Svelte and will feel at home there -- second argument is what the testing lib need -- don't worry much, you'll hardly ever use it", will become "there is one argument that is what you're used to, but not quite. Also if Svelte or the testing library change something it'll be a bucket of fun". I approve of the sentiment, but think this path leads to dragons.

@mcous mcous mentioned this issue Feb 15, 2024
Merged
@github-actions GitHub Actions
Copy link

🎉 This issue has been resolved in version 4.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@yanick yanick closed this as completed in 8d98aa5 Apr 18, 2024
@peterpeterparker peterpeterparker mentioned this issue Jun 29, 2024
Merged
peterpeterparker added a commit to dfinity/gix-components that referenced this issue Jul 1, 2024
@peterpeterparker @github-actions
test: update and migrate testing-library v5 (#455)
c0cb809 
# Motivation

Testing library
[v5.0.0](https://github.com/testing-library/svelte-testing-library/releases/tag/v5.0.0)
introduces a breaking change that changes the returned `container`
position (see issue
[#313](testing-library/svelte-testing-library#313)).

# Changes

- use `baseElement` instead of `container` when our test actually expect
the body to perform search
- adapt `rerender` which are now promises and do not expect the `props`
parameter

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
released on @next
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@yanick @mcous