Skip to content
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

Run the existing tests with deno #179

Closed
wants to merge 12 commits into from
Closed

Conversation

syhol
Copy link
Contributor

@syhol syhol commented Sep 23, 2023

This PR builds on the existing deno fix #178 and adds some new checks to make sure deno support does not break in future. You can see the diff against the #178 changes here: syhol/valibot@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: (DENO - VITEST) : Error running: deno -A --node-modules-dir npm:vitest denoland/deno#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

@netlify
Copy link

netlify bot commented Sep 23, 2023

‼️ Deploy request for valibot rejected.

Name Link
🔨 Latest commit f21e204

@fabian-hiller
Copy link
Owner

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

@syhol
Copy link
Contributor Author

syhol commented Sep 26, 2023

Rebased now #178 is merged. The diff looks much cleaner now.

@fabian-hiller
Copy link
Owner

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 fabian-hiller added the github GitHub related changes label Dec 5, 2023
@syhol
Copy link
Contributor Author

syhol commented Dec 12, 2023

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 #178. I believe this PR would have prevented that initial break as well as the cause of #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.

@fabian-hiller
Copy link
Owner

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.

@syhol
Copy link
Contributor Author

syhol commented Dec 14, 2023

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.

@fabian-hiller fabian-hiller added the priority This has priority label Dec 15, 2023
@syhol
Copy link
Contributor Author

syhol commented Dec 16, 2023

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

@fabian-hiller
Copy link
Owner

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

@syhol
Copy link
Contributor Author

syhol commented Mar 14, 2024

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

@fabian-hiller
Copy link
Owner

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
Copy link
Owner

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?

@syhol
Copy link
Contributor Author

syhol commented Jul 30, 2024

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.

@fabian-hiller
Copy link
Owner

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

@fabian-hiller
Copy link
Owner

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

@syhol
Copy link
Contributor Author

syhol commented Oct 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request github GitHub related changes priority This has priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants