valibot icon indicating copy to clipboard operation
valibot copied to clipboard

Run the existing tests with deno

Open syhol opened this issue 2 years ago • 14 comments

~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:

  1. 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 describe and it. This uses Deno.test under the hood and I'm not sure how much API parity it has with vitest, but it at least provides test.skip(...), test.only(...), before* and after* 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.
    • library/deno.json -> imports key: redirect import of vitest to instead use the denoVitestPolyfill.ts when run through the Deno runtime.
    • I put this file under testUtils to make it clear it's a testing utility and because src/comparison.ts could be moved into this folder in a future PR.
  2. Fix tests run under deno: Use optional chaining in record.test.ts and recordAsync.test.ts since unlike node, deno has no __proto__ property on objects.
  3. New test for mod.ts: A new test library/mod.test.ts that just checks that mod.ts can be imported.
  4. 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:

  1. The ability to run deno task test in the library directory to execute all existing tests with Deno
  2. All tests are now running and passing under Deno
  3. The tests will now fail if mod.ts fails to resolve dependencies
  4. The ability to run the Deno workflow on PRs

syhol avatar Sep 23 '23 08:09 syhol

Deploy request for valibot rejected.

Name Link
Latest commit f21e2042297a7d3ab1570e5564dbeef4d03327a9

netlify[bot] avatar Sep 23 '23 08:09 netlify[bot]

Thank you for your contribution! I will try to review your changes next week.

fabian-hiller avatar Sep 23 '23 17:09 fabian-hiller

Rebased now https://github.com/fabian-hiller/valibot/pull/178 is merged. The diff looks much cleaner now.

syhol avatar Sep 26 '23 12:09 syhol

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?

fabian-hiller avatar Dec 05 '23 03:12 fabian-hiller

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:

  1. Swap out the npm expect with the new deno expect from it's stdlib.
  2. Update the deno stdlib version
  3. Rebase the branch

Please let me know so I know whether to proceed.

syhol avatar Dec 12 '23 00:12 syhol

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.

fabian-hiller avatar Dec 12 '23 15:12 fabian-hiller

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.

syhol avatar Dec 14 '23 14:12 syhol

The PR is in a good state to be merged, let me know if there's anything more you need from me.

syhol avatar Dec 16 '23 02:12 syhol

Thank you! I will review it in the coming week or the week after.

fabian-hiller avatar Dec 16 '23 15:12 fabian-hiller

Now you're publishing to jsr, is it a good time to revisit this 😉 😉 ?

syhol avatar Mar 14 '24 10:03 syhol

I am currently very focused on the source code and documentation. Once v1 is released I will have a look at this PR.

fabian-hiller avatar Mar 14 '24 15:03 fabian-hiller

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?

fabian-hiller avatar Jul 30 '24 19:07 fabian-hiller

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.

syhol avatar Jul 30 '24 20:07 syhol

Yes, feel free to create a minimal PR. Thank you!

fabian-hiller avatar Jul 31 '24 08:07 fabian-hiller

We may switch to Deno v2 in the long run. Should I close this issue?

fabian-hiller avatar Oct 10 '24 03:10 fabian-hiller

Yes please, I've become quite busy recently. Sorry for the delay.

syhol avatar Oct 10 '24 21:10 syhol