test: refactor serverless invocations so in-process and sandbox implementation use shared code and ability to run multiple invocations in same sandbox #2871
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📊 Package size report No changes
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
pieh
commented
Apr 24, 2025
| @@ -1,3 +1,2 @@ | |||
| export * from './helpers.js' | |||
| export * from './mock-file-system.js' | |||
| export * from './stream-to-buffer.js' | |||
There was a problem hiding this comment.
this was never imported anywhere
pieh
commented
Apr 24, 2025
| @@ -6,7 +6,7 @@ import fg from 'fast-glob' | |||
| import { coerce, gt, gte, satisfies, valid } from 'semver' | |||
| import { execaCommand } from 'execa' | |||
|
|
|||
| const FUTURE_NEXT_PATCH_VERSION = '14.999.0' | |||
| const FUTURE_NEXT_PATCH_VERSION = '15.999.0' | |||
There was a problem hiding this comment.
unrelated to rest - it just impacts some conditional assertions depending on next versions when running tests locally and not setting RESOLVED_NEXT_VERSION env var (like CI does) - the idea of this const is to always use latest version conditionals when running locally as this is most likely what you'd test with
pieh
force-pushed
the
refactor/test-sandboxed-invocations
branch
2 times, most recently
from
863bc8d to
2b1d4a9
Compare
pieh
commented
Apr 24, 2025
| }) | ||
| const childProcess = spawn(process.execPath, [import.meta.dirname + '/sandbox-child.mjs'], { | ||
| stdio: ['pipe', 'pipe', 'pipe', 'ipc'], | ||
| cwd: join(ctx.functionDist, SERVER_HANDLER_NAME), |
There was a problem hiding this comment.
minor fix here to ensure CWD in sandboxes matches what we already do when invoking in process (with cwd mock there)
pieh
commented
Apr 24, 2025
Comment on lines
+538
to
+548
| /** | ||
| * Load function in child process and execute single invocation | ||
| */ | ||
| export async function invokeSandboxedFunction( | ||
| ctx: FixtureTestContext, | ||
| options: FunctionInvocationOptions = {}, | ||
| ) { | ||
| const { invokeFunction, exit } = await loadSandboxedFunction(ctx, options) | ||
| const result = await invokeFunction(options) | ||
| exit() | ||
| return result |
There was a problem hiding this comment.
this ensures existing tests using invokeSandboxedFunction continue to work as they used to
pieh
commented
Apr 24, 2025
Comment on lines
+117
to
+121
| const restoreEnvironment = temporarilySetEnv(ctx, env) | ||
|
|
||
| const { handler } = await import( | ||
| 'file:///' + join(ctx.functionDist, SERVER_HANDLER_NAME, '___netlify-entry-point.mjs') | ||
| ) |
There was a problem hiding this comment.
note that this changed order versus current implementation so that env vars are set BEFORE importing serverless handler
this is because some of the imported code evaluate env vars at import time - so it's pretty helpful to set them up early (for example when using NEXT_PRIVATE_DEBUG_CACHE)
…js module shared by both in-process and sandboxed invocations
pieh
force-pushed
the
refactor/test-sandboxed-invocations
branch
from
2b1d4a9 to
6b3a40a
Compare
pieh
commented
Apr 24, 2025
| @@ -2,11 +2,9 @@ import { assert, vi } from 'vitest' | |||
|
|
|||
| import { type NetlifyPluginConstants, type NetlifyPluginOptions } from '@netlify/build' | |||
| import { bundle, serve } from '@netlify/edge-bundler' | |||
| import type { LambdaResponse } from '@netlify/serverless-functions-api/dist/lambda/response.js' | |||
There was a problem hiding this comment.
this type was no longer even exported so I got this type in a bit different way in new place by doing some typescript transformations - it seems like we don't check types in test helpers generally (not something to address in this PR tho)
pieh
commented
Apr 24, 2025
Comment on lines
-29
to
-40
| export const createBlobContext = (ctx: FixtureTestContext) => | ||
| Buffer.from( | ||
| JSON.stringify({ | ||
| edgeURL: `http://${ctx.blobStoreHost}`, | ||
| uncachedEdgeURL: `http://${ctx.blobStoreHost}`, | ||
| token: BLOB_TOKEN, | ||
| siteID: ctx.siteID, | ||
| deployID: ctx.deployID, | ||
| primaryRegion: 'us-test-1', | ||
| }), | ||
| ).toString('base64') | ||
|
|
There was a problem hiding this comment.
moved to lambda-helpers.mjs (as it can't import from TS modules and does need to use it)
pieh
commented
Apr 24, 2025
| import { BLOB_TOKEN } from './constants.mjs' | ||
| import { execute as untypedExecute } from 'lambda-local' | ||
|
|
||
| const SERVER_HANDLER_NAME = '___netlify-server-handler' |
There was a problem hiding this comment.
this is copied from
(for the serverless invocation in same process it was imported from there, but it was previously already copied for sandboxed invocation because it can't import TS, so not fully new that this is duplicated)
pieh
commented
Apr 24, 2025
Comment on lines
+27
to
+42
| /** | ||
| * This is helper to get LambdaLocal's execute to actually provide result type instead of `unknown` | ||
| * Because jsdoc doesn't seem to have equivalent of `as` in TS and trying to assign `LambdaResult` type | ||
| * to return value of `execute` leading to `Type 'unknown' is not assignable to type 'LambdaResult'` | ||
| * error, this types it as `any` instead which allow to later type it as `LambdaResult`. | ||
| * @param {Parameters<typeof untypedExecute>} args | ||
| * @returns {Promise<LambdaResult>} | ||
| */ | ||
| async function execute(...args) { | ||
| /** | ||
| * @type {any} | ||
| */ | ||
| const anyResult = await untypedExecute(...args) | ||
|
|
||
| return anyResult | ||
| } |
There was a problem hiding this comment.
This is equavalent for untypedExecute() as Promise<LambdaResult>, but I couldn't find a way to do it nicer with jsdoc as simplest solution was causing Type 'unknown' is not assignable to type 'LambdaResult' ts errors
pieh
commented
Apr 24, 2025
Comment on lines
-1
to
-14
| /** | ||
| * Converts a readable stream to a buffer | ||
| * @param stream Node.js Readable stream | ||
| * @returns | ||
| */ | ||
| export function streamToBuffer(stream: NodeJS.ReadableStream) { | ||
| const chunks: Buffer[] = [] | ||
|
|
||
| return new Promise<Buffer>((resolve, reject) => { | ||
| stream.on('data', (chunk) => chunks.push(Buffer.from(chunk))) | ||
| stream.on('error', (err) => reject(err)) | ||
| stream.on('end', () => resolve(Buffer.concat(chunks))) | ||
| }) | ||
| } |
There was a problem hiding this comment.
moved to lambda-helpers.mjs
pieh
commented
Apr 24, 2025
Comment on lines
+14
to
+18
| 'message', | ||
| /** | ||
| * @param {any} msg | ||
| */ | ||
| async (msg) => { |
There was a problem hiding this comment.
This wasn't typed before, but adding explicit note was needed after adding // @ts-check to get at least partial type checking here and is this is not used directly in tests I didn't want to spend too much time on making details of sandbox messages fully typed
serhalp
approved these changes
Apr 25, 2025
Comment on lines
+3
to
+4
| // this is not TS file because it's used both directly inside test process | ||
| // as well as child process that lacks TS on-the-fly transpilation |
There was a problem hiding this comment.
I wonder if we could just use tsx or something? It's only for tests... Might be worth it for good type safety.
There was a problem hiding this comment.
We could try, but ideally this is one that matches what vitest is using and I wasn't sure it was worth the effort given TS-ish jsdocs are possibility. I've done something like that in gatsbyjs/gatsby#32120 (you can grep for ts-register there for adding on-the-fly TS support in tests, tho that exact solution was using @babel/register I wouldn't try to use here). But in gatsby case this child processes handling was part of core framework functionality so full type safety there was much more important, while here it might just result in our own test helper problems as we don't use this in actual next runtime
There was a problem hiding this comment.
but I'd leave this as maybe for the future, ideally we won't have to touch this code for a long time (I only touched sandbox code once before - when it was originally written heh and now I'm touching it only to add a feature to it)
|
|
||
| invokeFunctionImpl = await loadFunction(ctx, options) | ||
|
|
||
| if (process.send) { |
There was a problem hiding this comment.
when is this false? is this expected?
There was a problem hiding this comment.
typescript @types/node report it as nullable in general which forces some checking (due to added // @ts-check directive) - at least while in code editors, because we don't seem to actually enforce type checking in those test helpers
generally it won't exist in parent process or if there is no IPC bridge setup (this is to send IPC messages to parent process), but in our current usage it will actually always be defined
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This splits off changes needed for #2862 to be more self contained and to make things easier to review in chunks. It refactors serverless invocation helper for integration tests - it unify handling between
invokeFunctionandinvokeSandboxedFunctionto reuse same core code of invoking function - it moves needed parts to common JS file (because sandboxed version running in child process can't use TS, like main test process can - but for preserving type checking equiavalent jsdocs and// @ts-checkwas added). It also adds ability to keep sandbox running to allow invoking it multiple times while it preserve it's stateNotes for review:
Tests
This is changing integration test helper - all existing tests should pass
Relevant links (GitHub issues, etc.) or a picture of cute animal
Part of FRB-1405