test(cli): add reusable Vitest e2e helpers#217
Conversation
Signed-off-by: Yacine Kheddache <yacine@microcks.io>
yada
added
enhancement
lbroudoux
added
theme/cli
theme/build
chore
and removed
github_actions
lbroudoux
left a comment
lbroudoux
left a comment
There was a problem hiding this comment.
Some new stuff like cleanup, mcp-client or cli-runner makes sense, but I don't get the point of some other changes like the global fixtures that are not gloabl and the ability to restore infrastructure. Please review my comments.
| export const OPEN_METEO_SPEC = path.resolve(import.meta.dirname, '../../../dev/open-meteo-openapi.yml'); | ||
| export const OPEN_METEO_SERVICE_NAME = 'Open-Meteo APIs'; | ||
| export const OPEN_METEO_SERVICE_VERSION = '1.0'; | ||
| export const OPEN_METEO_BACKEND = 'https://api.open-meteo.com'; | ||
| export const OPEN_METEO_EXPECTED_TOOLS = 1; | ||
| export const OPEN_METEO_TOOL_NAME = 'get_v1_forecast'; |
There was a problem hiding this comment.
This looks like a global helper file but contains fixtures elements that are specific to one or more scenarios. This doesn't seem the right place to hold this kind of elements. I would have put this in a local fixture definition, next tot the scenarios.
There was a problem hiding this comment.
Agreed. The intent was to avoid duplicating constants across the planned follow-up e2e scenarios, but the Open-Meteo and (soon/next) GitHub values are scenario fixtures, not generic helper infrastructure. I’ll move those next to the scenario tests that need them and keep only truly shared e2e plumbing in helpers.
|
|
||
| export async function startInfrastructure(): Promise<void> { | ||
| console.log('🚀 Starting infrastructure via Docker Compose...'); | ||
| await recreateStackIfRequested(); |
There was a problem hiding this comment.
I don't get why we should recreate the stack here. startInfrastructure() is expected to be called once on an empty runner/environment. If the concern is to be able to run on a local development machine where the infra may be already there, then we should instead fail gracefully and require the developer to do some cleanup before launching the tests.
There was a problem hiding this comment.
This was added to make interrupted local reruns easier, but it changes the expected behavior of startInfrastructure() and is too implicit for this helper PR. CI should start from a clean runner, and local stale infrastructure should fail clearly so the developer can clean it up explicitly. I’ll remove the automatic stack recreation from this PR.
| return result; | ||
| } | ||
|
|
||
| export async function runCliJson<T = unknown>(...args: string[]): Promise<T> { |
There was a problem hiding this comment.
Is it an helper to run the CLI command with -o json flag? If yes, then we should add it explicitly and not set expectations on the caller to have added -o json to the method args.
There was a problem hiding this comment.
+1 The helper name implies that it runs a JSON-producing CLI command, so callers should not have to remember to pass -o json. I’ll update it so runCliJson() appends -o json itself and only accepts the logical CLI command arguments.
Thanks, that makes sense. I’ll keep the helper pieces that are genuinely reusable ( |
Signed-off-by: Yacine Kheddache <yacine@microcks.io>
|
@lbroudoux : PR updated, and I have also updated the Summary accordingly. |
2 participants
Summary:
This PR prepares the CLI Vitest e2e suite for broader coverage by extracting reusable helper utilities.
Changes:
-o jsonValidation:
Closes #216