Add local-runner for extraction testing and improve test loading#74
Add local-runner for extraction testing and improve test loading#74gasperzgonec wants to merge 9 commits intomainfrom
Conversation
radovanjorgic
left a comment
There was a problem hiding this comment.
I quickly reviewed and gave some comments, but I don't think this is the right direction. Approach is good (mock server, fixtures) but adding so much code in template is not what we want. We should think of abstracting this in SDK.
3870b8f to
6e64ff3
Compare
There was a problem hiding this comment.
Pull request overview
This PR replaces the credential-dependent local test runner with a fixture-based runner that uses @devrev/ts-adaas MockServer, enabling snap-in functions to be executed locally without needing real DevRev access.
Changes:
- Rewrote the test runner to load
event_context.json/state.json, resolve${ENV_VAR}placeholders, and run against aMockServer. - Updated the CLI to accept
--fixturePath(required) and optional--functionName,--local, and--printState. - Added example fixture directories and an
npm run fixturescript.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| code/src/test-runner/test-runner.ts | New fixture-driven runner that builds an AirdropEvent, starts/stops MockServer, injects fixture state, and supports env templating. |
| code/src/main.ts | Updates CLI args for fixture execution and injects positional local for SDK spawn local-mode logging. |
| code/package.json | Adds fixture script alias to run the fixture CLI via ts-node. |
| code/fixtures/start_extracting_external_sync_units/event_context.json | Example fixture event context for external sync unit extraction start. |
| code/fixtures/start_extracting_external_sync_units/state.json | Example state fixture for extraction. |
| code/fixtures/start_extracting_data/event_context.json | Example fixture event context for START_EXTRACTING_DATA. |
| code/fixtures/start_extracting_data/state.json | Example state fixture for extraction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…to AirdropMessage
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Please verify if fetch state returns exactly this object or full state object (with fields like lastSuccessfulSyncStarted, attachmentsMetadata and so on). We should have the full state, not sure if we should define only initial or full state here.
There was a problem hiding this comment.
The fetch state returns a full object, containing all of these values, if present.
There was a problem hiding this comment.
So if we have only {"foo": "bar"} in state.json, it will return full state object, with fields like lastSuccessfulSyncStared and attachmentsMetadata? Those fields will be overriden if explicitly specified in state.json file?
| @@ -0,0 +1,13 @@ | |||
| { | |||
| "event_type": "START_EXTRACTING_DATA", | |||
| "function_name": "extraction", | |||
There was a problem hiding this comment.
Hmm, if we name this event/airdrop message it is a bit confusing to have additional fields like function_name here. I would rather keep it as other snap-ins do - that you need to specify through args the function you want to test offline.
There was a problem hiding this comment.
While this is a fair observation, I don't think it's very reproducible. I'd like to have it be stored in the file, as one fixture will be either for extraction or loading, but never both (forcing this to be a CLI arg could add a new problem that could occur when we mismatch and push an extraction event into a loading function, which will never happen in the real life).
There was a problem hiding this comment.
I get the point but wanted to have clear separation of concerns here. If I am a developer, I would just copy the content of the real event into event.json and expect to be ready to test offline with it. Without having to add or understand additional fields like function_name. function_name sounds like "how to run" part of the process and is already a familiar dev flow for existing snap-ins (not adaas snapins).
Example from devrev-snaps-typescript-template README.md (https://github.com/devrev/devrev-snaps-typescript-template):
npm install
npm run start -- --functionName=on_work_creation --fixturePath=on_work_created_event.json
| "event_context": { | ||
| "external_sync_unit_id": "test_external_sync_unit_id" | ||
| } | ||
| } |
There was a problem hiding this comment.
Are all of these fields required? Are all required present here? For example I know request id field (or uuid form event context IIRC) is used when doing some requests, but I don't see it here.
There was a problem hiding this comment.
None of these fields are required, this file can be empty.
The defaults here override the missing values in there.
code/src/test-runner/test-runner.ts
Outdated
| * Fields from the fixture's airdrop_message.json are merged into the | ||
| * appropriate sections, with MockServer URLs always taking precedence. | ||
| */ | ||
| function buildEvent(mockServerBaseUrl: string, eventType: EventType, fixture?: FixtureAirdropMessage): AirdropEvent { |
There was a problem hiding this comment.
We have similar createEvent function in SDK, maybe we can export and use it here as well?
There was a problem hiding this comment.
SDK's createEvent is coupled to its internal Jest MockServer singleton — can't inject a custom base URL. Filed as future SDK enhancement.
There was a problem hiding this comment.
I would rather delay this and add what is needed to SDK to deliver simplified version of this, then: add to template, announce new feature -> people start pulling the same code, add to sdk -> tell people that this is outdated and can be removed.
| console.log(`[fixture] MockServer started on ${mockServer.baseUrl}`); | ||
|
|
||
| // Inject state from state.json (or default to empty state) | ||
| mockServer.setRoute({ |
There was a problem hiding this comment.
For now we can go with state only, but is there something else that user would like to override to test locally?
There was a problem hiding this comment.
Maybe we could add a different configuration options, for example, to support custom responses for some other endpoints (e.g: selective extraction itemTypes).
There was a problem hiding this comment.
Yes, we could do that.
| */ | ||
| export interface FixtureEvent { | ||
| event_type: string; | ||
| function_name?: FunctionFactoryType; |
There was a problem hiding this comment.
If we keep function_name (which I would not suggest), this will be required parameter, right?
| * Replaces `${VAR_NAME}` placeholders with values from `process.env`. | ||
| * Values are JSON-escaped so special characters don't break the JSON structure. | ||
| */ | ||
| function resolveEnvVariables(raw: string, filePath: string): string { |
There was a problem hiding this comment.
Oh okay so this enables using env variables in fixtures json files like this for example?
{
"foo": "${ENV_VAR_NAME}"?
}
There was a problem hiding this comment.
Where .env file should be located? In code/?
| throw new Error(`Unknown event_type "${input}". Must be one of: ${Object.values(EventType).join(', ')}`); | ||
| } | ||
|
|
||
| function buildEvent(mockServerBaseUrl: string, eventType: EventType, fixture?: FixtureEvent): AirdropEvent { |
There was a problem hiding this comment.
As said above, if possible let's move this to the sdk and reuse from there (if that is not harmful) and keep template minimal.
| console.log(`[fixture] MockServer started on ${mockServer.baseUrl}`); | ||
|
|
||
| // Inject state from state.json (or default to empty state) | ||
| mockServer.setRoute({ |
There was a problem hiding this comment.
Yes, we could do that.
There was a problem hiding this comment.
There is no state yet in start external sync units phase. It is stateless.
| /** Which function to run — overrides airdrop_message.json's function_name if set */ | ||
| functionName?: FunctionFactoryType; | ||
| /** Print the adapter state on every worker_data_url.update call */ | ||
| printState?: boolean; |
There was a problem hiding this comment.
Okay maybe saving to the file is not needed for now (since we don't have any orchestration) but then I think even --printState is not needed as flag. Anyone can just log the adapter.state at the end of process to see what the state looks like. And we anyhow have State is successfully updated to {...state} log from SDK at the end of extraction.
Fixture-based local testing with MockServer
Replaces the old test runner (which required real DevRev credentials) with a fixture-based system that uses the SDK's
MockServer. Snap-in functions can now be tested locally without any external dependencies.Changes
code/src/test-runner/test-runner.ts-- Full rewriteMockServer(from@devrev/ts-adaas) on a dynamic portstate.jsonfrom the fixture directory and injects it via theGET /worker_data_url.getroute overrideevent_context.jsonand builds a completeAirdropEventwith all SDK URLs (callback_url,worker_data_url,devrev_endpoint) pointing at the MockServer200 OKresponses automatically${ENV_VAR}templating in fixture JSON files -- variables are resolved fromprocess.env(with.envauto-loaded via dotenv). Missing variables produce a clear error message with the variable name and file path.addCredentials/ dotenv-PAT injection flowcode/src/main.ts-- Updated CLI interface--fixturePath(required): name of the fixture folder insidecode/fixtures/--functionName(optional): which function to run. Can also be set viafunction_nameinevent_context.json--local(optional): enables the SDK's local development mode -- log output is plain text messages instead of full JSON objects with event context tagscode/package.json"fixture"npm scriptcode/fixtures/-- Example fixturesstart_extracting_external_sync_units/-- event_context.json + state.jsonstart_extracting_data/-- event_context.json + state.jsonUsage
Creating new fixtures
Create a folder in
code/fixtures/<name>/with two files:event_context.json(onlyevent_typeis required):{ "event_type": "START_EXTRACTING_DATA", "function_name": "extraction", "connection_data": { "key": "${EXTERNAL_SYSTEM_TOKEN}", "key_type": "pat" } }state.json(optional -- defaults to empty state if missing):{ "todos": { "completed": false }, "users": { "completed": false }, "attachments": { "completed": false } }Environment variables can be provided via a
.envfile or exported in your shell.How it works
dotenv.config()loads.envintoprocess.env${VAR}placeholders are resolvedMockServerstarts on a random available portstate.jsoncontent is injected into the mockGET /worker_data_url.getendpointAirdropEventis constructed with test defaults and MockServer URLsNo real credentials or network access to DevRev is needed. External system calls (e.g. the
HttpClientin the template) still hit their configured endpoints as normal.Connected Issues
Checklist