valibot
valibot copied to clipboard
Run the existing tests with deno
~This PR builds on the existing deno fix https://github.com/fabian-hiller/valibot/pull/178 and adds some new checks to make sure deno support does not break in future. You can see the diff against the https://github.com/fabian-hiller/valibot/pull/178 changes here: https://github.com/syhol/valibot/compare/main...syhol:valibot:deno-tests~
What does this PR do:
- Vitest polyfill: Created a custom polyfill so Deno can run the vitest tests because vitest does not work in Deno today: https://github.com/denoland/deno/issues/19767.:
library/src/testUtils/denoVitestPolyfill.ts:- Uses Deno stdlib https://deno.land/[email protected]/testing/bdd.ts to provide
describeandit. This usesDeno.testunder the hood and I'm not sure how much API parity it has with vitest, but it at least providestest.skip(...),test.only(...),before*andafter*functions. - Uses npm https://www.npmjs.com/package/expect to provide
expect. This is the jest expect package because I can't get vitest expect to work for the same reason the vitest runner won't work; however, it should be API compatible as that's a design goal of vitest.
- Uses Deno stdlib https://deno.land/[email protected]/testing/bdd.ts to provide
library/deno.json->importskey: redirect import of vitest to instead use thedenoVitestPolyfill.tswhen run through the Deno runtime.- I put this file under testUtils to make it clear it's a testing utility and because
src/comparison.tscould be moved into this folder in a future PR.
- Fix tests run under deno: Use optional chaining in
record.test.tsandrecordAsync.test.tssince unlike node, deno has no__proto__property on objects. - New test for mod.ts: A new test
library/mod.test.tsthat just checks that mod.ts can be imported. - New GitHub workflow to run tests under Deno: We could also just reuse the NodeJs workflow, happy to change this. See the workflow passing in my fork here: https://github.com/syhol/valibot/actions/runs/6277943842/job/17050680986. I can't get it working on act (local github actions) for my M1 though.
What do we get out of this PR:
- The ability to run
deno task testin the library directory to execute all existing tests with Deno - All tests are now running and passing under Deno
- The tests will now fail if mod.ts fails to resolve dependencies
- The ability to run the Deno workflow on PRs
Deploy request for valibot rejected.
| Name | Link |
|---|---|
| Latest commit | f21e2042297a7d3ab1570e5564dbeef4d03327a9 |
Thank you for your contribution! I will try to review your changes next week.
Rebased now https://github.com/fabian-hiller/valibot/pull/178 is merged. The diff looks much cleaner now.
Sorry for not responding to this PR for so long. Is the idea to also run all tests with Deno to detect possible problems in advance?
Sorry for not responding to this PR for so long.
No problem at all @fabian-hiller
Is the idea to also run all tests with Deno to detect possible problems in advance?
Yes, exactly right. It looks like Valibot was initially compatible with Deno but at some point it was broken, we then fixed it in https://github.com/fabian-hiller/valibot/pull/178. I believe this PR would have prevented that initial break as well as the cause of https://github.com/fabian-hiller/valibot/pull/249
If you would be interested in progressing this PR then I would like to do a couple of things:
- Swap out the npm expect with the new deno expect from it's stdlib.
- Update the deno stdlib version
- Rebase the branch
Please let me know so I know whether to proceed.
I haven't made a decision yet, but I'm interested in this PR. Valibot seems to be one of the more popular packages with Deno, so it might make sense to make sure there are no problems here either when publishing a new version.
Swap out the npm expect with the new deno expect from it's stdlib.
I've been patching some missing behaviour but it looks like this is going to be hard migrate to until it supports asymmetric matchers.
I'm just going to do the first two for now.
The PR is in a good state to be merged, let me know if there's anything more you need from me.
Thank you! I will review it in the coming week or the week after.
Now you're publishing to jsr, is it a good time to revisit this 😉 😉 ?
I am currently very focused on the source code and documentation. Once v1 is released I will have a look at this PR.
Hey, since we are now publishing Valibot on JSR, it seems to me that this is no longer necessary. What do you think about it?
A lot of this PR can be deleted and cleaned up because Vitest is now supported in deno. But JSR won't run your tests for you so I still think you should run tests on Deno in GitHub actions, even more so now you claim to support Deno in JSR.
If you claim Valibot can run on Deno, then I recommend you run tests on Deno to verify that claim.
Would you like me to have a go at a simplified PR that just uses Deno to run the tests as they are? No shims, and as few changes as possible.
Yes, feel free to create a minimal PR. Thank you!
We may switch to Deno v2 in the long run. Should I close this issue?
Yes please, I've become quite busy recently. Sorry for the delay.