Skip to content

Fix render's binding to document.body instead of the container.#13

Closed
aranggitoar wants to merge 1 commit intosolidjs:mainfrom
aranggitoar:main
Closed

Fix render's binding to document.body instead of the container.#13
aranggitoar wants to merge 1 commit intosolidjs:mainfrom
aranggitoar:main

Conversation

@aranggitoar
Copy link
Copy Markdown

@aranggitoar aranggitoar commented Jul 22, 2022

Tested with,

test("the query container should not be document.body", () => {
  const { container, getAllByText } = render(() => <div>Some text...</div>);
  const falseContainer = document.createElement("p");
  falseContainer.textContent = "Some text...";
  container.parentNode.insertBefore(falseContainer, getAllByText("Some text...")[0].parentNode);
  expect(getAllByText("Some text...")[0] === container.childNodes[0]).toBe(true);
});

The test appends a false container with the same text as the rendered container. It is on the same level with the rendered container, but before the rendered container.

Then it checks if the queried element is the same with the child of container, which should be the original element. But before the fix it wouldn't be, as the query is bound to the document.body instead of the container. After the fix, it would be, as now the query is bound to the container instead of the document.body.

Before fix,
before-change

After fix,
after-change

@aranggitoar
Copy link
Copy Markdown
Author

Is this a good pull request format? If there is any standard to comply, please let me know. 😄

@atk
Copy link
Copy Markdown
Collaborator

atk commented Nov 6, 2022

First of all, the PR misses the motivation for the change. Since solid-testing-library is ported from preact-testing-library, it has the same behavior. However, this behavior can confuse developers who are familiar with @testing-library/react, which indeed queries from the container and not from baseElement. The rationale is that one is supposed to use screen instead - however, that is still an option in any case while porting tests from react might result in hard-to-find errors; therefore I wholeheartedly support your motivation.

As for the rest of the PR, there's not much meat. Why not directly add the test case, if you already wrote it?

In any case, since i moved the testing to vitest and updated a few things, I ended up implementing this myself based on your work.

@atk atk closed this Nov 6, 2022
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