flarelabs-net / vite-plugin-cloudflare

12 stars 1 forks source link

shamefully copy vite's testing structure #41

Closed dario-piotrowicz closed 3 weeks ago

dario-piotrowicz commented 4 weeks ago

This PR updates our testing structure to the same one the vite monorepo does, which as I mentioned I find quite convenient to work with (given how much this project is bound to vite following their testing structure also feels appropriate)

Noticeable changes:

jamesopstad commented 3 weeks ago

I haven't yet added any testing for builds, I did start but I wasn't sure what the best approach for running wrangler would be, my preference, after chatting with @RamIdeas would to be wait for his PR to merge (or have a properly working prerelease) and use the startMultiWorker from there. So I'd leave testing builds to a followup PR if that's ok (also since this PR is changing a lot of files I'd prefer to merge it sooner rather than later)

I don't think we'll be able to do much in the way of testing for builds until we've decided what the build output format should be for configuration (the two options are a wrangler.json file per worker or the newly proposed build output API).

Will we need to run Wrangler to test the builds like you suggest? I was imagining that we would run Miniflare with the bundled output provided as the modules to the workers array.

dario-piotrowicz commented 3 weeks ago

I don't think we'll be able to do much in the way of testing for builds until we've decided what the build output format should be for configuration (the two options are a wrangler.json file per worker or the newly proposed build output API).

I can point startMultiWorker to our src toml files and override the entries to the vite built js files (instead of the src ones), in that way we can kind of get a starting rudimental build testing up and running.

It's not super polished or anything and very likely just a temporary solution to get a minimal test coverage for builds

I totally agree with you that before having something definitive/proper we'd have to decide what output format we'll want.

Will we need to run Wrangler to test the builds like you suggest? I was imagining that we would run Miniflare with the bundled output provided as the modules to the workers array.

That's a good suggestion, I can look into that if we'd want to use that, although I'd have to read the configs manually and stitch things together etc... that's why I was looking into leveraging something that wrangler already exports (or will soon) instead (especially since I am looking at a simple temporary solution for now)

dario-piotrowicz commented 3 weeks ago
  • It seems like the new tests are mainly derived from the old examples rather than the old fixtures (correct me if I'm wrong). I tried to be consistent about returning JSON in the fixtures and wonder if that might be better because it's a bit of a mixture of strings and JSON at the moment. Just thinking that the fewer choices we have to make when we write tests, the easier it will be.

Yes they are more derived from the old examples, I leaned on those because one of the main points of this change is to have some apps clear to play with/debug/hack around that then also serve as tests, compared to the other way around (having something created in a more standard/"rigid" way that can also be debugged).

I appreciate trying to be consistent but in this case I wouldn't necessarily bother, especially since later I imagine that we'll have all sort of applications also returning html etc... so trying to make all fit the same structure/mold doesn't seem too necessary/worth it to me.

This being said, if you definitely prefer that I return JSONs in all the existing apps I'd be ok with that πŸ™‚

  • I'm not massively keen on using snapshots for testing and would prefer if we were explicit about what's expected. Don't mind too much though so happy to use snapshots if you prefer.

I think snapshots are pretty cool, but a bit if of a gimmick to me honest πŸ˜…, I'm don't have a strong preference regarding using them or not, since you do prefer not to have them let me just remove them πŸ™‚πŸ‘

dario-piotrowicz commented 3 weeks ago

This all looks good to me. I think maybe just remove the ~utils aliasing and extra tsconfigs as we discussed earlier (we can always add this aliasing in again in the future).

Thanks πŸ˜„

Yeah I've removed the aliasing yesterday, I just didn't have the change to push my changes up πŸ™‚πŸ‘