Skip to content

Conversation

@flakey5
Copy link
Member

@flakey5 flakey5 commented Jan 16, 2025

Alternative title: making the tests & local dev environment not annoying and messy

This is an attempt to make the tests and local dev environment a lot nicer to work with.

Some of the main problems with them right now are:

  1. No Sentry = no error visibility, nothing is logged to console
  2. You can't run the worker in development unless it's in wrangler's remote mode. This requires a Cloudflare account & a R2 bucket to be setup on that account which is annoying
  3. Tests are rather messy and haven't really been updated like the rest of the codebase has been. This was good when we were changing a lot so we could ensure we didn't break anything, but, it's time now to clean these up imo

This pr:

  1. Rewrites tests to be a lot cleaner & conform with the style of the rest of the codebase
  2. Adds an example of the structure of the dist-prod bucket (called dev-bucket)
    • Replaces the bucket data at tests/e2e/test-data/R2_BUCKET for e2e tests
    • Can be used in a dev script to populate a bucket for local testing (see 3)
  3. npm run dev - starts a locally running version of the worker with a populated R2 bucket
    • Populated from the dev-bucket directory

What's TODO:

  • Finish implementing e2e tests
  • Go through code to see if there's anything that should be tested that isn't
  • Make the local dev script nicer

Some current annoyances:

  • The local dev script uses Miniflare since there isn't actually a way to populate an R2 bucket locally with Wrangler, so any niceness that that has we need to reimplement or go without. Hot reloading might not be possible either since Miniflare works off of the compiled worker.js file. I do have some ideas, but not entirely sure if they're gonna work.
  • Ideally we would be able to use Workers' vitest integration, but it's not really possible since we're doing things like mocking an S3 api in our e2e tests. Vitest patches a lot of modules and globals making things like node:http unavailable. Workaround for this was found, so we can use the Vitest integration now

@flakey5 flakey5 requested a review from a team as a code owner January 16, 2025 07:10
@flakey5 flakey5 marked this pull request as draft January 16, 2025 07:10
@flakey5 flakey5 force-pushed the flakey5/20241222/tests branch from cabb03d to 7f020e2 Compare January 27, 2025 20:45
@ovflowd
Copy link
Member

ovflowd commented Feb 24, 2025

@flakey5 -- do you need a review now or is this still WIP?

@flakey5
Copy link
Member Author

flakey5 commented Feb 24, 2025

Still a wip

@flakey5 flakey5 force-pushed the flakey5/20241222/tests branch 4 times, most recently from 74763d5 to 2d28a66 Compare April 27, 2025 02:15
@flakey5 flakey5 force-pushed the flakey5/20241222/tests branch 4 times, most recently from 47f78f7 to 91d7f42 Compare April 27, 2025 02:37
@flakey5
Copy link
Member Author

flakey5 commented Apr 27, 2025

Gonna go ahead and mark this for review now.

This unfortunately doesn't resolve the annoyances with running the worker locally though, there's still some things that I need to play around with to see if I can get it working in an ideal way.

@flakey5 flakey5 marked this pull request as ready for review April 27, 2025 03:34
},
});

if (env.LOG_ERRORS === true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be exported to somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdym?

files: Record<string, File>;
}

async function listDirectory(directoryPath: string): Promise<Directory> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the contents of the dev-bucket folder and loading them into memory for the tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOC, why all these .gitkeep files? Is this like a "test" recration of a folder structure? Instead of this, could we make GHA create all these folders with mkdir before tests? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this like a "test" recration of a folder structure

Yeah, this serves two purposes:

  1. Helps with making local dev easier since a structurally accurate example is provided. This is especially useful for newcomers or for those that don't have R2 access. Ideally this will be reused in the local dev script I mentioned in the pr description but haven't gotten around to making it work yet. This is also why I think we shouldn't generate these files via GHA
  2. Makes managing the contents of the bucket used during tests easier since it's not just a bunch of binary files

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me so long to review this and for that, I'm sorry! I left a few comments, but the PR looks good :)

@ovflowd
Copy link
Member

ovflowd commented Aug 6, 2025

cc @flakey5 thanks for bearing with me 🙇

Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
@flakey5 flakey5 force-pushed the flakey5/20241222/tests branch from 91d7f42 to 0d252c4 Compare September 13, 2025 19:30
@flakey5 flakey5 requested a review from ovflowd September 13, 2025 19:30
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com>
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM!

@ovflowd
Copy link
Member

ovflowd commented Sep 16, 2025

@flakey5 OOC are you switching from Wrangler to the Vite Plugin?

@ovflowd
Copy link
Member

ovflowd commented Sep 16, 2025

Ah sorry, nvm, you just added Vitest. Forgot of that.

@flakey5 flakey5 merged commit 8dda19a into main Sep 16, 2025
5 checks passed
@flakey5 flakey5 deleted the flakey5/20241222/tests branch September 16, 2025 17:05
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.

4 participants