-
Notifications
You must be signed in to change notification settings - Fork 79
feat: fixed test setup #240
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
base: main
Are you sure you want to change the base?
Conversation
|
Having ported and maintaining the Qwik Testing Library, I would also be happy if it could be published under the It could be nice to do it before proceeding with this PR so it follows the same package namespace as the other frameworks 👀 Let me know! |
src/__tests__/qwik.js
Outdated
| `<h1>This is the title</h1> | ||
| <p>Here's a <!--qv --><span class="strong"><!--qv q:key q:sref=0 q:s-->neat<!--/qv--></span><!--/qv--> demo:</p> | ||
| <!--qv --><div>mdx-bundler with Qwik's runtime!</div><!--/qv-->` | ||
| ) |
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 would understand if we want to keep it that way, but to prevent having to update the test once we migrate to qwik V2, we could use queries to match the expected results instead of the raw inner html.
For example:
const element = await render(
Qwik.jsx(Component, { components: { strong: SpanBold } })
)
assert.ok(element.getByRole("heading", { name: "This is the title" }))
assert.ok(element.getByText("Here's a neat demo: mdx-bundler with Qwik's runtime!"))Or something along those lines.
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.
Yeah this should also work, since the v1 import is compatible in v2. I'll update this soon 🫡 . Appreciate the feedback!
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.
Actually @ianlet I'm getting some linter issues from the react testing library that it's bad practice to not destructure here.
But when I try to use screen, it becomes hard to select the h1 for the qwik smoke test
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.
Ah no worries! In any case, as we're testing the result of a transformation, it makes sense to expect the exact output. It's just that it makes this test more fragile and that we'll have to remember to come fix it as soon as Qwik internals change something (shouldn't happen often though).
kentcdodds
left a comment
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! However, I'm not certain I want to include tests for every possible framework in this package. Feels a little unnecessary and adds a burden I would rather not have.
Hey Kent! Completely understand. In that case this PR can be used as inspiration to fix the test setup surrounding the existing tests 🙂 . Let me know if you need me to update the readme. I think there is a part that mentions how mdx bundler works with multiple frameworks, perhaps we can add something in there to prevent future outstanding PR's assuming this package is trying to test agnostic capabilities. One last thing, is there anything we can do to change the scope on the qwik testing library package from noma.to to If not no worries, I'll have to use playwright and vitest more in that case rather than testing library 🤔 |
|
If you would like to get included into the testing library org, I recommend going to the testing library Discord and asking the current maintainers whether that package can be moved into the testing library org. I would be happy to accept this PR with some of the testing improvements that you've made without the Qwik related stuff. Thanks! |
Just removed the Qwik part! Regarding testing library, unfortunately I did not get a response from any of the maintainers. I guess I could start looking more into other test tooling. |
kentcdodds
left a comment
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!
|
Hmmm... The tests are failing :( |
|
Darn! I'll take a look after work |

What:
This PR tackles:
npm run test).Why:
It has been an absolute pleasure to start contributing to this library. I want to express my sincere gratitude to @kentcdodds for both being a significant inspiration and for his invaluable React teachings that have helped shape my development journey.
This PR is a small way to give back to the community, and showcasing MDX Bundler's ability to support any JSX framework.
The Qwik testing library package has been added, it would be awesome (and also fix my OCD), if the Qwik testing library can be moved under the
@testing-librarynamespace.How:
These changes were implemented by using Qwik testing library, along with the
jsdom-globalpackage.jsdom-globalplays a big role in other testing tools under the hood, an example being vitest which takes many of the underlying code of this package, we are able to better handle tests and also remove somets-expect-errorlinesChecklist:
Minimal reproduction of the event issue in main without
jsdom-global:Error blocking CI:
#239