Conversation
…d add integration tests for pagination beyond CI_PAGE_SIZE
There was a problem hiding this comment.
Pull request overview
Refactors the CLI’s search implementations to page through all results (instead of being capped by default/CI page sizes), and adds an integration test that verifies search pd / list pd return >1000 process definitions.
Changes:
- Updated multiple
search*command functions to usefetchAllPages()for cursor-based pagination. - Added an integration test that deploys >1000 process definitions and asserts CLI returns all results in JSON mode.
- Added a mini BPMN fixture used as a template for bulk deployments in the pagination test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/commands/search.ts |
Switches search endpoints to fetchAllPages() so results aren’t truncated by page size limits. |
tests/integration/pagination.test.ts |
New integration test that deploys >1000 definitions and validates pagination via CLI search pd / list pd. |
tests/fixtures/mini-process.bpmn |
New BPMN template fixture used by the pagination integration test. |
Comments suppressed due to low confidence (3)
tests/integration/pagination.test.ts:112
allFoundis assigned but never used. Either remove the variable or assert on it (e.g.,assert.ok(allFound, ...)) to make timeouts/failures easier to diagnose.
// Poll until Elasticsearch has indexed all deployed definitions
const allFound = await pollUntil(async () => {
const result = cli('search', 'pd', '--id=mini-process-*');
if (result.status !== 0) return false;
try {
const items = JSON.parse(result.stdout);
return Array.isArray(items) && items.length >= DEPLOY_COUNT;
} catch {
return false;
}
}, POLL_TIMEOUT_MS, POLL_INTERVAL_MS);
tests/integration/pagination.test.ts:145
allFoundis assigned but never used. Either remove it or assert it so the test fails with a clear message when the polling times out.
// Still poll briefly in case the test order changes.
const allFound = await pollUntil(async () => {
const result = cli('list', 'pd');
if (result.status !== 0) return false;
try {
const items = JSON.parse(result.stdout);
return Array.isArray(items) && items.length >= DEPLOY_COUNT;
} catch {
return false;
}
}, POLL_TIMEOUT_MS, POLL_INTERVAL_MS);
tests/integration/pagination.test.ts:94
- The deployment is implemented as its own
test(), but the later tests implicitly depend on it having run successfully. To avoid ordering/coupling issues, consider moving the deploy step into thebefore()hook (or into the same test that asserts pagination) so each test can be run independently.
test(`deploy ${DEPLOY_COUNT} process definitions via CLI`, { timeout: SPAWN_TIMEOUT_MS }, () => {
const result = cli('deploy', bpmnDir);
assert.strictEqual(
result.status, 0,
`Deploy should exit 0. stderr: ${result.stderr}`,
);
});
| const allItems = await fetchAllPages( | ||
| (f, opts) => client.searchProcessInstances(f, opts), | ||
| filter, | ||
| hasCiFilter ? CI_PAGE_SIZE : undefined, | ||
| ); |
There was a problem hiding this comment.
fetchAllPages's pageSize parameter is typed as number, but this passes undefined when hasCiFilter is false. In strict mode this won’t compile. Either call fetchAllPages without the pageSize argument when you want the default, or provide a concrete number.
| const allItems = await fetchAllPages( | |
| (f, opts) => client.searchProcessInstances(f, opts), | |
| filter, | |
| hasCiFilter ? CI_PAGE_SIZE : undefined, | |
| ); | |
| const allItems = hasCiFilter | |
| ? await fetchAllPages( | |
| (f, opts) => client.searchProcessInstances(f, opts), | |
| filter, | |
| CI_PAGE_SIZE, | |
| ) | |
| : await fetchAllPages( | |
| (f, opts) => client.searchProcessInstances(f, opts), | |
| filter, | |
| ); |
src/commands/search.ts
Outdated
| const allItems = await fetchAllPages( | ||
| (f, opts) => client.searchJobs(f, opts), | ||
| filter, | ||
| hasCiFilter ? CI_PAGE_SIZE : undefined, |
There was a problem hiding this comment.
This call passes undefined for pageSize when hasCiFilter is false, but fetchAllPages declares pageSize as a required number parameter (with a default). Under strict TS that’s a type error. Fix by omitting the argument or using an explicit default value.
| const allItems = await fetchAllPages( | |
| (f, opts) => client.searchJobs(f, opts), | |
| filter, | |
| hasCiFilter ? CI_PAGE_SIZE : undefined, | |
| const allItems = await (hasCiFilter | |
| ? fetchAllPages( | |
| (f, opts) => client.searchJobs(f, opts), | |
| filter, | |
| CI_PAGE_SIZE, | |
| ) | |
| : fetchAllPages( | |
| (f, opts) => client.searchJobs(f, opts), | |
| filter, | |
| ) |
| const allItems = await fetchAllPages( | ||
| (f, opts) => client.searchVariables({ ...f, truncateValues }, opts), | ||
| filter, | ||
| hasCiFilter ? CI_PAGE_SIZE : undefined, | ||
| options.limit, | ||
| ); |
There was a problem hiding this comment.
Two strict-mode typing issues here: (1) pageSize is typed as number in fetchAllPages, but hasCiFilter ? CI_PAGE_SIZE : undefined can be undefined; and (2) options.limit is number | undefined but fetchAllPages's maxItems parameter is typed as number. Either omit these args when not set, or coalesce them to concrete numbers before passing.
src/commands/search.ts
Outdated
| const allItems = await fetchAllPages( | ||
| (f, opts) => client.searchProcessDefinitions(f, opts), | ||
| filter, | ||
| hasCiFilter ? CI_PAGE_SIZE : undefined, |
There was a problem hiding this comment.
fetchAllPages expects a number for pageSize, but this call passes undefined when hasCiFilter is false. With strict TypeScript settings this is a type error and will break npm run build. Consider omitting the pageSize argument when you want the default, or pass an explicit number (e.g. API_DEFAULT_PAGE_SIZE).
| const allItems = await fetchAllPages( | |
| (f, opts) => client.searchProcessDefinitions(f, opts), | |
| filter, | |
| hasCiFilter ? CI_PAGE_SIZE : undefined, | |
| const allItems = await (hasCiFilter | |
| ? fetchAllPages( | |
| (f, opts) => client.searchProcessDefinitions(f, opts), | |
| filter, | |
| CI_PAGE_SIZE, | |
| ) | |
| : fetchAllPages( | |
| (f, opts) => client.searchProcessDefinitions(f, opts), | |
| filter, | |
| ) |
src/commands/search.ts
Outdated
| const allItems = await fetchAllPages( | ||
| (f, opts) => client.searchUserTasks(f, opts), | ||
| filter, | ||
| hasCiFilter ? CI_PAGE_SIZE : undefined, |
There was a problem hiding this comment.
This passes undefined into fetchAllPages(..., pageSize) when hasCiFilter is false, but pageSize is typed as number in fetchAllPages. With strict TS this is a compile-time error. Consider conditionally omitting the argument instead of passing undefined.
| const allItems = await fetchAllPages( | |
| (f, opts) => client.searchUserTasks(f, opts), | |
| filter, | |
| hasCiFilter ? CI_PAGE_SIZE : undefined, | |
| const allItems = await (hasCiFilter | |
| ? fetchAllPages( | |
| (f, opts) => client.searchUserTasks(f, opts), | |
| filter, | |
| CI_PAGE_SIZE, | |
| ) | |
| : fetchAllPages( | |
| (f, opts) => client.searchUserTasks(f, opts), | |
| filter, | |
| ) |
src/commands/search.ts
Outdated
| const allItems = await fetchAllPages( | ||
| (f, opts) => client.searchIncidents(f, opts), | ||
| filter, | ||
| hasCiFilter ? CI_PAGE_SIZE : undefined, |
There was a problem hiding this comment.
fetchAllPages's pageSize parameter is number; passing undefined here (when hasCiFilter is false) will fail TypeScript compilation under strict. Prefer calling fetchAllPages with only the required args when you want the default page size.
| const allItems = await fetchAllPages( | |
| (f, opts) => client.searchIncidents(f, opts), | |
| filter, | |
| hasCiFilter ? CI_PAGE_SIZE : undefined, | |
| const allItems = await ( | |
| hasCiFilter | |
| ? fetchAllPages((f, opts) => client.searchIncidents(f, opts), filter, CI_PAGE_SIZE) | |
| : fetchAllPages((f, opts) => client.searchIncidents(f, opts), filter) |
…SON mode; adjust tests accordingly
|
This has been released in 2.4.1-alpha.2. |
|
This has been released in 2.4.1. |
No description provided.