refactor: split remote handler + add unit tests#16064
refactor: split remote handler + add unit tests#16064elliott-with-the-longest-name-on-github wants to merge 1 commit into
Conversation
|
| } | ||
| }) | ||
| ); | ||
| import { create_mock_event } from '../../../test/mocks/server.js'; |
There was a problem hiding this comment.
can we create an internal import for these like #test/mocks/server?
|
|
||
| vi.mock(import('@sveltejs/kit/internal/server'), async (actualPromise) => { | ||
| const actual = await actualPromise(); | ||
| const { create_mock_event, create_mock_state } = await import('../../../test/mocks/server.js'); |
There was a problem hiding this comment.
why do we need a static import and a dynamic one?
| // @ts-expect-error | ||
| globalThis.__SVELTEKIT_DEV__ = false; |
There was a problem hiding this comment.
depending on how often we find ourselves doing this it might be worth creating a helper for it
| import { | ||
| create_mock_event, | ||
| create_mock_internals, | ||
| create_mock_manifest, | ||
| create_mock_options, | ||
| create_mock_remote, | ||
| create_mock_request, | ||
| create_mock_state | ||
| } from '../../../../test/mocks/server.js'; |
There was a problem hiding this comment.
might feel nicer like this:
| import { | |
| create_mock_event, | |
| create_mock_internals, | |
| create_mock_manifest, | |
| create_mock_options, | |
| create_mock_remote, | |
| create_mock_request, | |
| create_mock_state | |
| } from '../../../../test/mocks/server.js'; | |
| import * as mocks from '#test/mocks/server'; |
followed by
const internals = mocks.create_internals(...);
| import { record_span } from '../../telemetry/record_span.js'; | ||
| import { handle_error_and_jsonify } from '../utils.js'; | ||
| import { collect_remote_data } from './collect.js'; | ||
| import { run_command, run_form, run_prerender, run_query, run_query_batch } from './dispatch.js'; |
There was a problem hiding this comment.
what's the argument for separating call.js and dispatch.js? wouldn't it be easier to put the contents of dispatch.js in here?
|
this is probably going to cause some merge conflict misery — it might be better to wait until more of the PR queue gets merged before going much further. any PRs targeting |
|
Overall I'm happy that we're moving in the direction of faster tests, but after a quick first pass through I worry about a couple of things (besides the aforementioned merge conflicts):
Obviously what we have now is a PITA and I'm not defending it. But I think the sweet spot is much more coarse-grained than this — for example it would be nice to test something like 'given an app with the following manifest, assert that if I call |
2 participants
I'm on a slow-and-steady mission to start being able to replace some of our integration tests with unit tests. Our integration tests often contort themselves into all sorts of horrible shapes to try to assert that some internal-to-sveltekit or side-effect-y thing is happening. Part of this effort is going to be busting up some of these big "orchestrator" files into abstractions that are easier to test. The remote function server handling hadn't gotten too bad yet, but it was definitely on its way there. I split it up some:
remote.jsinto more-focusedremote/modules. I think most of the code is clearer now.test/mocks/server.js-- we had a couple of places where we were partially mock-and-cast-ing our request state stuff, this just kinda standardizes itrequested()limits, and OTEL span emissionUNIT-MIGRATION-CANDIDATEfor later; when I've had time to do this to more of the codebase and we're still stable for a while, we can sweep through and remove them