test(unenv-preset): move tests from nodejs-hybrid-app #7700

vicb · 2025-01-08T14:44:34Z

Move the tests from nodejs-hybrid-app

The tests are executed with the dev version of @cloudflare/unenv-preset and the pkg local version of workerd (instead of the pinned versions that will be used by wrangler).

Note that currently wrangler still uses the cloudflare preset from unjs/unenv so that is what is tested for now. Once wrangler switches to using @cloudflare/unenv-preset the resolution override will be in effect (we are trying to resolve $clouflare.ts files that are not in @cloudflare/unenv-preset)

Tests are disabled on Windows as MINIFLARE_WORKERD_PATH is not supported.

/cc @pi0

Tests
- TODO (before merge)
- Tests included
- Tests not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required
- Not required because: not affected
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Documentation not necessary because: no user facing change

changeset-bot · 2025-01-08T14:44:39Z

⚠️ No Changeset found

Latest commit: 1ae5839

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

packages/unenv-preset/tests/build.ts

github-actions · 2025-01-08T15:05:05Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12692667361/npm-package-wrangler-7700

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7700/npm-package-wrangler-7700

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12692667361/npm-package-wrangler-7700 dev path/to/script.js

Additional artifacts:

wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12692667361/npm-package-cloudflare-workers-bindings-extension-7700 -O ./cloudflare-workers-bindings-extension.0.0.0-v7e2886048.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v7e2886048.vsix

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12692667361/npm-package-create-cloudflare-7700 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12692667361/npm-package-cloudflare-kv-asset-handler-7700

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12692667361/npm-package-miniflare-7700

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12692667361/npm-package-cloudflare-pages-shared-7700

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12692667361/npm-package-cloudflare-unenv-preset-7700

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12692667361/npm-package-cloudflare-vitest-pool-workers-7700

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12692667361/npm-package-cloudflare-workers-editor-shared-7700

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12692667361/npm-package-cloudflare-workers-shared-7700

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12692667361/npm-package-cloudflare-workflows-shared-7700

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.100.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20241230.0
`workerd`	1.20241230.0	1.20241230.0
`workerd --version`	1.20241230.0	2024-12-30

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

petebacondarwin

I'm a bit confused about the testing strategy here.

It seems that you are compiling a Worker in order to then run it via wrangler dev. Is that right? Why do that rather than just point Wrangler at the Worker directly, telling it to use the different unenv preset?

petebacondarwin · 2025-01-09T09:33:11Z

packages/unenv-preset/tests/index.test.ts

+		if (isWindows) {
+			return;
+		}


Can you not just use describe.skipIf(isWindows) rather than adding this in every hook and test?

Actually, is it not possible to make these tests work on Windows?
You say that MINIFLARE_WORKERD_PATH is not supported on Windows. Is that strictly accurate or is it that you just need to specify a different path for Windows (e.g. "node_modules/.bin/workerd.cmd" or "node_modules/.bin/workerd.ps1")?

Can you not just use describe.skipIf(isWindows) rather than adding this in every hook and test?

TIL - that's much nicer, thanks for the tip.

Actually, is it not possible to make these tests work on Windows?

That's what I saw in miniflare tests - see the link in the PR description.
I don't think it would add much value anyway as we are already testing on Max + Ubuntu?

I think that comment in Miniflare is not the same thing. What they are saying there (I believe) is that their test uses a shebang binary to trigger workerd, which would not work on Windows. But when you install workerd it should provide a Windows compatible binary, which usually ends in .cmd or .ps1, and that ought to work in this scenario.

I am not convinced that testing on Max+Ubuntu is adequate to ensure Windows compatibility 😆

I am not convinced that testing on Max+Ubuntu is adequate to ensure Windows compatibility 😆

What I mean is that I don't really care about the dev version working on Windows because I don't think any use of dev on Windows. Wrangler is tested on windows which should be what matters.

But I'll see if there is something I can do without spending too much time.

petebacondarwin · 2025-01-09T09:51:12Z

packages/unenv-preset/tests/build.ts

+
+console.log(pkgDir);
+
+execSync(`node ${wranglerPath} deploy --dry-run --outdir ./dist/worker`, {


Sorry to be dumb but why are we building a Worker here?

That the same thing we do for nodejs-hybrid-app in the "build": "wrangler deploy --dry-run --outdir=./dist",

That is we need to serve the worker for the tests.

We have to execSync here to override the resolution base folders (via WRANGLER_UNENV_RESOLVE_PATHS).

That's a good question, I'll add a comment to the file.

In the fixture I believe Igor added the build script so that when we run pnpm build at the root of the monorepo it will run the build on that fixture and check that the build completes. Other than that we don't use it at all. But moreover "building" the fixture makes sense - it literally is the Worker.

But here, I feel this is conflating testing with the package itself. The build script should be building the preset package not the fixture used for testing it. To be honest I would rather we kept all these tests in the nodejs-hybrid-app fixture and enabled being able to build and test that fixture against the workspace builds of the preset rather than partially duplicating the tests across the package and the fixture.

vicb · 2025-01-09T10:21:03Z

I'm a bit confused about the testing strategy here.

It seems that you are compiling a Worker in order to then run it via wrangler dev. Is that right? Why do that rather than just point Wrangler at the Worker directly, telling it to use the different unenv preset?

Yep that should work too.

I only did what nodejs-hybrid-app does without thinking too much.

edit: At least it's easy to inspect the generated file to check that resolution works well

packages/unenv-preset/tests/build.ts

petebacondarwin · 2025-01-09T11:09:20Z

OK. So I forgot that we have this problem with when we bump the unenv version in Wrangler.
I see what you are trying to do now, which is to allow us to test bleeding edge unenv+workerd before we bump the version in Wrangler.

I think we should drop that extra build from the preset package build command, and allow the developer to run it independently if they want. Or if we want to actually have that as a test do the build as part of a test.

I still think we should try to test on Windows. While it might not be part of a CF developer's daily workflow, it would be annoying to find that an unenv preset change breaks on Windows only after we try to bump the preset version in Wrangler.

petebacondarwin

A couple of suggestions.

packages/unenv-preset/wrangler.json

packages/unenv-preset/tests/index.test.ts

petebacondarwin · 2025-01-09T13:28:35Z

fixtures/nodejs-hybrid-app/src/index.ts

@@ -29,8 +28,6 @@ export default {
 				return testX509Certificate();
 			case "/test-require-alias":
 				return testRequireUenvAliasedPackages();
-			case "/test-unenv-preset":
-				return await testUnenvPreset();


Given that the testing strategy is now to test local changes to the unenv preset in the package, it might be worth keeping this test here so that we also get test coverage on the published unenv preset?

I think it makes sens to have preset related tests in the @cloudflare/unenv-preset and in a single location (I don't think we will think about updating 2 places especially because they should not be updated at the same time)

An improvement I want to do (as a follow up) is to test both the stable and dev versions from the preset package. This is why I asked James and Yagiz about figuring out the workerd version at runtime (to only run applicable tests). It sounds like it is not possible but we can still pass the version via the request.

packages/unenv-preset/tests/index.test.ts

vicb · 2025-01-09T15:11:43Z

@petebacondarwin thanks a lot for your patience and insights with this review. I ended up disabling windows tests (adding a comment with the link to the relevant Node issue). Let me know if there is more to do here.

🙏

IgorMinar · 2025-01-09T16:57:41Z

yay!

MORTEZAMIRSALI · 2025-01-10T11:16:31Z

7c8ae1c

vicb requested a review from a team as a code owner January 8, 2025 14:44

github-advanced-security bot found potential problems Jan 8, 2025

View reviewed changes

packages/unenv-preset/tests/build.ts Fixed Show fixed Hide fixed

vicb added the no-changeset-required label Jan 8, 2025

vicb requested a review from petebacondarwin January 8, 2025 16:10

vicb force-pushed the preset-tests branch from 6b2d960 to 2178a90 Compare January 8, 2025 20:49

petebacondarwin reviewed Jan 9, 2025

View reviewed changes

vicb requested a review from petebacondarwin January 9, 2025 10:16

vicb commented Jan 9, 2025

View reviewed changes

packages/unenv-preset/tests/build.ts Outdated Show resolved Hide resolved

vicb force-pushed the preset-tests branch from c250029 to e86c7a9 Compare January 9, 2025 10:31

vicb force-pushed the preset-tests branch 2 times, most recently from 5985bd3 to d1b2ffe Compare January 9, 2025 12:03

petebacondarwin approved these changes Jan 9, 2025

View reviewed changes

petebacondarwin reviewed Jan 9, 2025

View reviewed changes

packages/unenv-preset/tests/index.test.ts Outdated Show resolved Hide resolved

vicb force-pushed the preset-tests branch 2 times, most recently from 1522194 to 625937e Compare January 9, 2025 14:48

test(unenv-preset): move tests from nodejs-hybrid-app

1ae5839

vicb force-pushed the preset-tests branch 3 times, most recently from 67bd34c to 1ae5839 Compare January 9, 2025 15:07

vicb requested a review from petebacondarwin January 9, 2025 15:11

anonrig approved these changes Jan 9, 2025

View reviewed changes

vicb merged commit b8e5f63 into main Jan 9, 2025
29 checks passed

vicb deleted the preset-tests branch January 9, 2025 15:45

penalosa pushed a commit that referenced this pull request Jan 10, 2025

test(unenv-preset): move tests from nodejs-hybrid-app (#7700)

6124c6f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(unenv-preset): move tests from nodejs-hybrid-app #7700

test(unenv-preset): move tests from nodejs-hybrid-app #7700

vicb commented Jan 8, 2025 •

edited

Loading

changeset-bot bot commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

petebacondarwin left a comment

petebacondarwin Jan 9, 2025

petebacondarwin Jan 9, 2025

vicb Jan 9, 2025

petebacondarwin Jan 9, 2025

vicb Jan 9, 2025

petebacondarwin Jan 9, 2025

vicb Jan 9, 2025

petebacondarwin Jan 9, 2025

vicb commented Jan 9, 2025 •

edited

Loading

petebacondarwin commented Jan 9, 2025

petebacondarwin left a comment

petebacondarwin Jan 9, 2025

vicb Jan 9, 2025

vicb commented Jan 9, 2025

IgorMinar commented Jan 9, 2025

MORTEZAMIRSALI commented Jan 10, 2025


		console.log(pkgDir);

		execSync(`node ${wranglerPath} deploy --dry-run --outdir ./dist/worker`, {

test(unenv-preset): move tests from nodejs-hybrid-app #7700

test(unenv-preset): move tests from nodejs-hybrid-app #7700

Conversation

vicb commented Jan 8, 2025 • edited Loading

changeset-bot bot commented Jan 8, 2025 • edited Loading

⚠️ No Changeset found

github-actions bot commented Jan 8, 2025 • edited Loading

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented Jan 9, 2025 • edited Loading

petebacondarwin commented Jan 9, 2025

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented Jan 9, 2025

IgorMinar commented Jan 9, 2025

MORTEZAMIRSALI commented Jan 10, 2025

vicb commented Jan 8, 2025 •

edited

Loading

changeset-bot bot commented Jan 8, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading

vicb commented Jan 9, 2025 •

edited

Loading