Skip to content

chore: add initial tests to repo #52

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

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Conversation

brettkolodny
Copy link
Collaborator

Closes #45

The initials tests in this PR cover basic UX and form rendering. There are some questions around how much really needs to be tested within this repo as testing everything would lead to a lot of duplicate with the tests that already exist within coder/coder.

Going forward I think the best ways to increase test coverage for this repo are:

  • Figure out how to actually run the wasm code within tests
    • Currently all of the wasm code is mocked and the preview is being tested on static outputs. This limits our ability to test the dynamic part of dynamic parameters. Vite has some wasm plugins but the way Go operates seems to complicate this slightly.
  • Work on a more graceful way to share coder between coder/coder and other repos
    • The actual components for dynamic parameters are being vendored from coder/coder with minimal change. It'd be nice if instead of this code being copy-pasted and then modified we had a better system for keeping the two code bases in sync. This would help with confidence in terms of tests in coder/coder applying to this repo.

Copy link

vercel bot commented Aug 4, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 4, 2025 4:03pm

@brettkolodny brettkolodny marked this pull request as ready for review August 4, 2025 16:03
@Kira-Pilot
Copy link
Member

Kira-Pilot commented Aug 4, 2025

@brettkolodny, can you please chunk this up so we can review? 2k+ LOC is a lot to look at in one shot and you'll get a more meaningful review if your diffs are smaller, e.g. ~500 LOC.

@brettkolodny
Copy link
Collaborator Author

brettkolodny commented Aug 4, 2025

It looks scarier than it is but the majority of those line changes are in the pnpm lock file which can essentially be ignored as it should mostly be from the new packages added in the package.json.

I was having some pnpm issues after after a workspace update which could lead to big lockfile changes if I accidentally changed the pnpm version but I'm not seeing a change in the lockfile version so I think it's correct?

@Kira-Pilot
Copy link
Member

It looks scarier than it is but the majority of those line changes are in the pnpm lock file which can essentially be ignored as it should mostly be from the new packages added in the package.json.

That makes sense! Seems like a 1k+ line diff even discounting the lockfile so I would still recommend chunking it.

I was having some pnpm issues after after a workspace update which could lead to big lockfile changes if I accidentally changed the pnpm version but I'm not seeing a change in the lockfile version so I think it's correct?

Looks like we're not pinning a pnpm version so maybe we could try that (in a separate PR) to avoid drift in the future.

@brettkolodny
Copy link
Collaborator Author

Will do! Going to move this PR to draft now and separate it out into two different PRs

@brettkolodny brettkolodny marked this pull request as draft August 5, 2025 16:17
@brettkolodny
Copy link
Collaborator Author

First PR, I don't love having branches/PRs off of other branches that aren't main so I'll open up the next one after this one is submitted

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.

Add tests
2 participants