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

chore: integration tests setup #116

Merged
merged 2 commits into from
Oct 7, 2024
Merged

chore: integration tests setup #116

merged 2 commits into from
Oct 7, 2024

Conversation

fforbeck
Copy link
Member

@fforbeck fforbeck commented Oct 2, 2024

Integration Test Setup for Cloudflare Local Worker

In this PR, I have implemented an integration test setup to verify the functionality of our Cloudflare Worker. This setup involves running the worker locally with all the bindings enabled in wrangler.toml (integration environment), and performing a series of HTTP requests to ensure the worker behaves as expected under various conditions.

By running the worker locally, we avoid the complexities and overhead associated with handling authentication for deploying a remote worker via CI. This makes the testing process more straightforward.

Key Components

  1. Worker Fixture:

    • Purpose: The fixture file contains the setup and teardown logic for the Cloudflare Worker used in our tests.
    • Functions:
      • setupWorker: Deploys the worker and starts the local Wrangler dev server.
      • teardownWorker: Stops the local Wrangler dev server.
      • getWorkerState: Provides the current state of the worker, including IP, port, and output.
  2. Integration Test Folder:

    • Purpose: This folder will contain the actual test files with logic to verify the worker's various functionalities.

Configuration

  • Worker Configuration: The worker configuration is defined in the wrangler.toml file. This includes settings such as rate limiting, environment variables, and other configurations necessary for the worker to function correctly. Make sure you are editing the integration environment config - which is the default environment used by the integration test setup.

@fforbeck fforbeck force-pushed the integration-tests branch 3 times, most recently from a3f1a35 to 4e211c4 Compare October 3, 2024 12:33
Peeja
Peeja previously requested changes Oct 4, 2024
Copy link
Member

@Peeja Peeja left a comment

Choose a reason for hiding this comment

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

Season 3 Nbc

test/integration/rate-limit.spec.js Show resolved Hide resolved
* The IP address of the test worker.
* @type {string}
*/
let ip = 'localhost'
Copy link
Member

Choose a reason for hiding this comment

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

suggestion Maybe let's put these all in one object (the return value of await runWranglerDev()), to make it clear that they all get set together. I had to read through the whole file a couple of times to understand what was setting these.

Along those lines, let's separate the requested port (which is a constant) from the actual port, and stop providing default values for ip and port. Right now, they're lying until Wrangler boots. If Wrangler's not up yet, we should signal that rather than provide fake values.

One way to set this up (which might be overkill) would be to have mochaGlobalSetup() store the promise from runWranglerDev() rather than await it itself, and have getWorkerInfo() be async. Then the tests will start booting Wrangler and not wait for it to be set up until the values are actually requested. (We don't really have a performance issue here, though, so the choice should be down to which one communicates what's happening the clearest.)

package.json Outdated Show resolved Hide resolved
test/unit/middlewares/rate-limiter.spec.js Show resolved Hide resolved
/** @type {(value: { ip: string port: number }) => void} */
let resolveReadyPromise
/** @type {(reason: unknown) => void} */
let rejectReadyPromise
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Let's avoid all these lets and put the stuff below inside the new Promise(). It'll have access to resolve and reject, and we don't need to track settledReadyPromise at all: resolveing or rejecting after it's settled is a noop.

test/helpers/run-wrangler.js Show resolved Hide resolved
test/helpers/run-wrangler.js Outdated Show resolved Hide resolved
@fforbeck fforbeck requested a review from Peeja October 4, 2024 18:55
@@ -18,7 +18,7 @@
"build": "esbuild --bundle src/index.js --format=esm --sourcemap --minify --outfile=dist/worker.mjs && npm run build:tsc",
"build:debug": "esbuild --bundle src/index.js --format=esm --outfile=dist/worker.mjs",
"build:tsc": "tsc --build",
"test:miniflare": "npm run build:debug && mocha --experimental-vm-modules test/index.spec.js",
"test:miniflare": "npm run build:debug && mocha --experimental-vm-modules --recursive test/miniflare/**/*.spec.js",
Copy link
Member

Choose a reason for hiding this comment

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

praise: I like this move!

@fforbeck fforbeck dismissed Peeja’s stale review October 7, 2024 12:11

Most of the suggestions were implemented. The open ones won't be addressed for now.

@fforbeck fforbeck merged commit 9444729 into main Oct 7, 2024
1 check passed
@fforbeck fforbeck deleted the integration-tests branch October 7, 2024 12:22
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.

3 participants