More E2E entity creation flows that trigger jobs #581
Conversation
WalkthroughThis update refactors the end-to-end testing infrastructure by replacing the Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant EntitiesBuilder
participant EntityRefs
participant ApiClient
TestSuite->>EntitiesBuilder: upsertSystem()
EntitiesBuilder->>ApiClient: Create/Update System
ApiClient-->>EntitiesBuilder: Response
EntitiesBuilder->>EntityRefs: Store system reference
TestSuite->>EntitiesBuilder: upsertResources()
EntitiesBuilder->>ApiClient: Create/Update Resources
ApiClient-->>EntitiesBuilder: Response(s)
EntitiesBuilder->>EntityRefs: Store resource references
TestSuite->>EntitiesBuilder: upsertDeployments()
EntitiesBuilder->>ApiClient: Create/Update Deployments
ApiClient-->>EntitiesBuilder: Response(s)
EntitiesBuilder->>EntityRefs: Store deployment references
TestSuite->>EntitiesBuilder: cleanupImportedEntities()
EntitiesBuilder->>ApiClient: Delete Entities
ApiClient-->>EntitiesBuilder: Delete Responses
EntitiesBuilder-->>TestSuite: Array of FetchResultInfo
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
# Conflicts: # e2e/api/entities-builder.ts # e2e/api/entity-fixtures.ts # e2e/api/schema.ts # e2e/tests/api/deployment-remove-event.spec.ts # e2e/tests/api/deployment-variable.spec.ts # e2e/tests/api/deployment-version.spec.ts # e2e/tests/api/deployments.spec.ts # e2e/tests/api/environments.spec.ts # e2e/tests/api/job-agents.spec.ts # e2e/tests/api/jobs/job-flow-1.spec.ts # e2e/tests/api/jobs/job-flow-entities.spec.yaml # e2e/tests/api/policies/matched-release-targets.spec.ts # e2e/tests/api/release-targets.spec.ts # e2e/tests/api/release.spec.ts # e2e/tests/api/resource-grouping.spec.ts # e2e/tests/api/resource-relationships.spec.ts # e2e/tests/api/resource-variables.spec.ts # e2e/tests/api/resources.spec.ts # e2e/tests/api/yaml-import.spec.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (3)
e2e/tests/api/policies/matched-release-targets.spec.ts (3)
452-452:⚠️ Potential issueFix undefined variable reference.
The variable
importedEntitiesis not defined. This should usebuilder.refsconsistent with the refactor.Apply this diff:
- const systemPrefix = importedEntities.system.slug.split("-")[0]!; + const systemPrefix = builder.refs.system.slug.split("-")[0]!;
510-510:⚠️ Potential issueFix undefined variable references throughout the file.
Multiple references to undefined
importedEntitiesvariable need to be updated to usebuilder.refsto be consistent with the refactor.Apply this diff pattern throughout:
- const systemPrefix = importedEntities.system.slug.split("-")[0]!; + const systemPrefix = builder.refs.system.slug.split("-")[0]!;Also applies to: 586-586, 664-664, 747-747, 853-853, 936-936, 1042-1042, 1125-1125, 1231-1231, 1359-1359
593-593:⚠️ Potential issueFix system ID reference.
The reference to
importedEntities.system.idshould usebuilder.refs.system.idto be consistent with the refactor.Apply this diff:
- systemId: importedEntities.system.id, + systemId: builder.refs.system.id,
🧹 Nitpick comments (3)
e2e/tests/api/deployment-variable.spec.ts (1)
216-216: Fix typo in test name.There's a typo in the test name: "shoudl" should be "should".
- test("shoudl fail if more than one default value is provided", async ({ api }) => { + test("should fail if more than one default value is provided", async ({ api }) => {e2e/api/entity-refs.ts (1)
164-169: Consider adding the item details to the error message.The error message could be more helpful by including information about what was being searched for.
function exactlyOne<T>(items: Array<T>): T { if (items.length !== 1) { - throw new Error(`Expected exactly one item, but found ${items.length}.`); + throw new Error(`Expected exactly one item, but found ${items.length} items.`); } return items[0]!; }Additionally, consider updating the calling methods to provide context:
public getEnvironmentLike(match: string): EnvironmentRef { const filtered = this.environments.filter((env) => matches(env.name, match)); if (filtered.length !== 1) { throw new Error(`Expected exactly one environment matching "${match}", but found ${filtered.length}.`); } return filtered[0]; }e2e/api/entities-builder.ts (1)
9-12: Consider improving type safety for the interface.The use of
anytypes inFetchResponse<any, any, any>reduces type safety. Consider using generic type parameters to maintain type information from the API calls.-export interface FetchResultInfo { - fetchResponse: FetchResponse<any, any, any>; - requestBody?: any; -} +export interface FetchResultInfo<TData = any, TError = any, TBody = any> { + fetchResponse: FetchResponse<TData, TError, TBody>; + requestBody?: TBody; +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
e2e/README.md(1 hunks)e2e/api/entities-builder.ts(1 hunks)e2e/api/entity-refs.ts(1 hunks)e2e/package.json(0 hunks)e2e/tests/api/deployment-remove-event.spec.ts(5 hunks)e2e/tests/api/deployment-variable.spec.ts(9 hunks)e2e/tests/api/deployment-version.spec.ts(8 hunks)e2e/tests/api/deployments.spec.ts(16 hunks)e2e/tests/api/environments.spec.ts(15 hunks)e2e/tests/api/job-agents.spec.ts(1 hunks)e2e/tests/api/jobs/job-flow-1.spec.ts(1 hunks)e2e/tests/api/jobs/job-flow-entities.spec.yaml(1 hunks)e2e/tests/api/policies/matched-release-targets.spec.ts(7 hunks)e2e/tests/api/release-targets.spec.ts(3 hunks)e2e/tests/api/release.spec.ts(11 hunks)e2e/tests/api/resource-grouping.spec.ts(1 hunks)e2e/tests/api/resource-relationships.spec.ts(1 hunks)e2e/tests/api/resource-variables.spec.ts(10 hunks)e2e/tests/api/resources.spec.ts(10 hunks)e2e/tests/api/yaml-import.spec.ts(4 hunks)e2e/tests/api/yaml-import.spec.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- e2e/package.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
e2e/tests/api/release-targets.spec.tse2e/tests/api/job-agents.spec.tse2e/tests/api/resource-grouping.spec.tse2e/tests/api/deployment-variable.spec.tse2e/tests/api/resource-relationships.spec.tse2e/tests/api/deployment-remove-event.spec.tse2e/tests/api/deployment-version.spec.tse2e/tests/api/yaml-import.spec.tse2e/tests/api/deployments.spec.tse2e/tests/api/environments.spec.tse2e/tests/api/policies/matched-release-targets.spec.tse2e/tests/api/release.spec.tse2e/tests/api/resource-variables.spec.tse2e/tests/api/resources.spec.tse2e/tests/api/jobs/job-flow-1.spec.tse2e/api/entities-builder.tse2e/api/entity-refs.ts
🧬 Code Graph Analysis (1)
e2e/tests/api/resource-variables.spec.ts (3)
e2e/tests/fixtures.ts (1)
test(11-26)e2e/api/entities-builder.ts (1)
cleanupImportedEntities(340-389)packages/db/src/schema/workspace.ts (1)
workspace(18-27)
🔇 Additional comments (34)
e2e/tests/api/resource-grouping.spec.ts (1)
13-14: LGTM! Method naming improved to reflect upsert semantics.The change from
createSystem()andcreateResources()toupsertSystem()andupsertResources()aligns with the refactor objectives and provides better semantics for idempotent operations that can handle both creation and updates.e2e/README.md (1)
58-63:❓ Verification inconclusive
Documentation updated to reflect new function signature.
The addition of the
tracerparameter aligns with the enhanced tracking capabilities introduced in this refactor.Verify that all existing usages of
importEntitiesFromYamlhave been updated to include the newtracerparameter:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining usages of importEntitiesFromYaml with the old 3-parameter signature # Expected: All calls should include the tracer parameter rg -A 3 "importEntitiesFromYaml\(" --type tsLength of output: 46
🏁 Script executed:
#!/bin/bash # Search for any calls to importEntitiesFromYaml in TS/JS/TSX files to ensure the tracer parameter is included rg -n "importEntitiesFromYaml\(" --glob '*.ts' --glob '*.js' --glob '*.tsx' -C2Length of output: 81
Manual verification required: confirm all importEntitiesFromYaml calls include the new tracer parameter
The previous search returned no matches, which may be due to the function only being invoked in non-.ts/.js files or multi-line contexts. Please double-check manually that every usage—regardless of file extension or call style—has been updated to pass the new
tracerargument. For example, you can run:# Broad search across all files and extensions rg -n "importEntitiesFromYaml" -C2Ensure that any invocation you find follows the updated four-parameter signature:
await importEntitiesFromYaml(api, workspace.id, yamlPath, tracer);e2e/tests/api/deployment-remove-event.spec.ts (3)
15-15: LGTM! Consistent method rename to upsert semantics.The change from
createSystem()toupsertSystem()is consistent with the refactor pattern seen across other test files.
19-19: LGTM! Cleanup updated to use new EntityRefs class.The change from
builder.cachetobuilder.refsaligns with the refactor to use the new EntityRefs class for entity management.
27-27: LGTM! Consistent entity reference updates across all test methods.All system entity references have been consistently updated from
builder.cache.systemtobuilder.refs.system, maintaining uniformity across the test suite.Also applies to: 113-113, 201-201, 290-290, 376-376
e2e/tests/api/release-targets.spec.ts (3)
14-17: LGTM! Consistent refactoring to upsert methods.The changes from
create*toupsert*methods align with the broader refactoring to useEntityRefsand provide enhanced entity tracking capabilities.
21-21: LGTM! Updated cleanup to use new EntityRefs.The cleanup function signature correctly updated to use
builder.refsinstead ofbuilder.cache, consistent with the EntityRefs refactoring.
30-30: LGTM! Consistent entity access via refs.All entity access patterns consistently updated from
builder.cachetobuilder.refs, maintaining the same functionality while using the new EntityRefs interface.Also applies to: 66-72
e2e/tests/api/job-agents.spec.ts (2)
15-17: LGTM! Consistent refactoring to upsert methods.The setup methods correctly updated to use the new
upsert*variants, aligning with the EntityRefs refactoring.
21-21: LGTM! Updated cleanup to use EntityRefs.Cleanup function correctly updated to use
builder.refs, consistent with the new entity management approach.e2e/tests/api/deployment-variable.spec.ts (3)
15-16: LGTM! Consistent setup method refactoring.Setup methods correctly updated to use
upsert*variants, aligning with the EntityRefs refactoring.
21-21: LGTM! Updated cleanup to use EntityRefs.Cleanup correctly updated to use
builder.refsinstead ofbuilder.cache.
25-25: LGTM! Consistent entity access via refs.All deployment entity access patterns consistently updated from
builder.cache.deployments[0]tobuilder.refs.deployments[0], maintaining functionality while using the new EntityRefs interface.Also applies to: 72-72, 150-150, 217-217, 261-261, 390-390, 496-496, 627-627, 714-714
e2e/tests/api/resource-variables.spec.ts (4)
15-17: LGTM! Consistent setup method refactoring.Setup methods correctly updated to use
upsert*variants, aligning with the EntityRefs refactoring.
21-21: LGTM! Updated cleanup to use EntityRefs.Cleanup correctly updated to use
builder.refsinstead ofbuilder.cache.
25-25: LGTM! Consistent system entity access via refs.All system slug access patterns consistently updated from
builder.cache.system.slugtobuilder.refs.system.slug, maintaining functionality while using the new EntityRefs interface.Also applies to: 80-80, 141-141, 174-174, 269-269, 421-421, 596-596
326-326: LGTM! Consistent deployment entity access via refs.All deployment entity access patterns consistently updated from
builder.cache.deployments[0]tobuilder.refs.deployments[0], maintaining functionality while using the new EntityRefs interface.Also applies to: 481-481, 655-655
e2e/tests/api/resource-relationships.spec.ts (1)
16-16: LGTM! Clean refactoring from EntitiesCache to EntityRefs.The changes consistently update entity references from
builder.cachetobuilder.refsand method calls fromcreate*toupsert*semantics. This aligns well with the broader refactoring described in the PR objectives.Also applies to: 18-19, 23-23
e2e/tests/api/deployment-version.spec.ts (2)
15-16: LGTM! Consistent refactoring to new entity management system.The setup and cleanup methods have been properly updated to use the new
upsert*methods andEntityRefsinterface.Also applies to: 20-20
33-33: LGTM! All entity references consistently updated.All deployment ID references have been properly migrated from
builder.cache.deployments[0].idtobuilder.refs.deployments[0].id.Also applies to: 46-46, 61-61, 84-84, 107-107, 154-154, 195-195
e2e/tests/api/yaml-import.spec.ts (3)
4-4: LGTM! Import statement properly simplified.The import has been correctly updated to use the unified API export.
14-19: Verify thecreateDeploymentVariablesmethod.Most entity creation methods have been updated to use
upsert*semantics, butcreateDeploymentVariables()remains unchanged. Please verify if this is intentional (e.g., noupsertDeploymentVariablesmethod exists) or if this should also be updated.
27-27: LGTM! All entity references consistently migrated.All entity access points have been properly updated from
builder.cachetobuilder.refsthroughout the test cases.Also applies to: 33-33, 69-69, 72-72, 88-88, 90-90
e2e/tests/api/deployments.spec.ts (2)
15-17: LGTM! Setup and cleanup properly refactored.The test setup has been correctly updated to use
upsert*methods and the newEntityRefsinterface for cleanup.Also applies to: 21-21
30-30: LGTM! Comprehensive entity reference migration.All entity references throughout the test cases have been systematically updated from
builder.cachetobuilder.refs. The changes maintain consistency across system IDs, environment lists, resource lists, and system slugs.Also applies to: 46-46, 74-74, 108-108, 115-115, 201-201, 207-207, 251-251, 265-265, 275-275, 281-281, 354-355, 369-369, 380-380, 409-409, 456-456, 462-462, 508-508, 536-536
e2e/tests/api/resources.spec.ts (3)
15-17: LGTM! Entity creation methods properly updated.The refactor from
createX()toupsertX()methods is correctly applied and aligns with the newEntityRefspattern.
21-21: LGTM! Cleanup method properly updated.The change from
builder.cachetobuilder.refsis correctly applied for the cleanup function.
25-25: LGTM! Entity references consistently updated.All entity references have been correctly updated from
builder.cachetobuilder.refsthroughout the test suite. The refactor is consistently applied.Also applies to: 52-53, 103-103, 137-137, 166-166, 215-216, 238-238, 285-285, 339-340, 368-368
e2e/tests/api/environments.spec.ts (2)
15-17: LGTM! Entity setup methods properly refactored.The transition from
createX()toupsertX()methods is correctly implemented and consistent with the new entity management pattern.
21-21: LGTM! Entity references consistently updated throughout.All references to entities have been properly updated from the old
builder.cachepattern to the newbuilder.refspattern. The refactor is thoroughly and consistently applied.Also applies to: 29-29, 43-43, 82-82, 96-96, 112-112, 149-149, 157-157, 198-199, 213-213, 224-224, 228-228, 260-260, 290-290, 310-310, 334-334, 341-341, 430-430, 457-457
e2e/tests/api/release.spec.ts (2)
15-17: LGTM! Entity setup methods correctly updated.The refactor from
createX()toupsertX()methods is properly implemented and aligns with the new entity management approach.
22-22: LGTM! Entity references consistently refactored.All entity references have been correctly updated from
builder.cachetobuilder.refsthroughout the test suite. The refactor is comprehensively applied with no missed references.Also applies to: 30-30, 36-36, 53-53, 113-113, 119-119, 166-166, 230-230, 236-236, 270-270, 334-334, 340-340, 471-471, 477-477, 589-589, 595-595, 718-718, 724-724, 847-847, 853-853
e2e/tests/api/policies/matched-release-targets.spec.ts (1)
35-35: LGTM! These entity references are correctly updated.These references properly use
builder.refsand are consistent with the refactor pattern.Also applies to: 93-93, 169-169, 241-241, 299-299, 375-375, 381-381
e2e/api/entity-refs.ts (1)
175-185: LGTM! Clean implementation of Fisher-Yates shuffle.The
takeRandomfunction correctly implements the Fisher-Yates shuffle algorithm with proper bounds checking and uses modern array destructuring for swapping.
| @@ -109,4 +109,4 @@ policies: | |||
| key: tier | |||
| value: production | |||
| versionAnyApprovals: | |||
| requiredApprovalsCount: 2 | |||
| - requiredApprovalsCount: 2 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the data structure change for versionAnyApprovals.
The change from a single object to an array containing one object suggests a schema evolution to support multiple approval configurations. Please confirm this aligns with the expected API schema and doesn't break existing functionality.
Verify the policy API schema accepts the new array structure:
🏁 Script executed:
#!/bin/bash
# Description: Check how versionAnyApprovals is used in policy-related code
# Expected: Code should handle array structure for versionAnyApprovals
rg -A 5 -B 5 "versionAnyApprovals" --type tsLength of output: 45059
versionAnyApprovals remains a single object – array in YAML spec will break schema
Our investigation shows that throughout the codebase versionAnyApprovals is defined and persisted as a single nullable object, not an array:
- In packages/db/src/schema/policy-relations.ts:
versionAnyApprovals: one(policyRuleAnyApproval) - In packages/rule-engine/src/types.ts:
versionAnyApprovals: schema.PolicyRuleAnyApproval | null - In the policy‐creation and update routines, and in API router queries, it’s always treated as one object.
Changing your E2E YAML to use an array will not match the runtime model and will break import and validation. You should either:
-
Revert the YAML spec back to:
versionAnyApprovals: requiredApprovalsCount: 2
or
-
Fully migrate
versionAnyApprovalsto a list by updating:
• packages/db/src/schema/policy-relations.ts (usemany(...))
• packages/db/src/schema/policy.ts, types in rule-engine, manager functions, create/update handlers
• API router return types and OpenAPI specs
• SDK and E2E schemas
Please choose one path to keep the test and implementation in sync.
🤖 Prompt for AI Agents
In e2e/tests/api/yaml-import.spec.yaml at line 112, the versionAnyApprovals
field was changed from a single object to an array, which conflicts with the
current API schema that expects a single nullable object. To fix this, revert
the YAML back to using a single object for versionAnyApprovals with
requiredApprovalsCount: 2, or alternatively, if you want to support multiple
approvals, update the entire codebase schema, types, handlers, API specs, and
SDK to handle an array consistently. Choose one approach to keep the test and
implementation aligned.
| builder.upsertSystem(); | ||
| builder.upsertResources(); | ||
| builder.upsertEnvironments(); | ||
| builder.upsertDeployments(); |
There was a problem hiding this comment.
Add missing await keywords for async operations.
The entity creation methods are async and need to be awaited to ensure proper setup before tests run.
Apply this diff to fix the missing await keywords:
- builder.upsertSystem();
- builder.upsertResources();
- builder.upsertEnvironments();
- builder.upsertDeployments();
+ await builder.upsertSystem();
+ await builder.upsertResources();
+ await builder.upsertEnvironments();
+ await builder.upsertDeployments();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| builder.upsertSystem(); | |
| builder.upsertResources(); | |
| builder.upsertEnvironments(); | |
| builder.upsertDeployments(); | |
| await builder.upsertSystem(); | |
| await builder.upsertResources(); | |
| await builder.upsertEnvironments(); | |
| await builder.upsertDeployments(); |
🤖 Prompt for AI Agents
In e2e/tests/api/policies/matched-release-targets.spec.ts around lines 17 to 20,
the calls to builder.upsertSystem(), builder.upsertResources(),
builder.upsertEnvironments(), and builder.upsertDeployments() are asynchronous
but missing the await keyword. Add await before each of these method calls to
ensure they complete before proceeding, guaranteeing proper test setup.
| - name: "{{ prefix }}-qa-database-resource" | ||
| <<: *kindService | ||
| identifier: "{{ prefix }}-qa-database-resource" |
There was a problem hiding this comment.
Fix incorrect kind anchors for database resources.
The database resources are using *kindService anchor but should use *kindDatabase anchor to match their names and intended purpose.
Apply this diff to fix the incorrect anchor references:
- name: "{{ prefix }}-qa-database-resource"
- <<: *kindService
+ <<: *kindDatabase
identifier: "{{ prefix }}-qa-database-resource" - name: "{{ prefix }}-dev-database-resource"
- <<: *kindService
+ <<: *kindDatabase
identifier: "{{ prefix }}-dev-database-resource"Also applies to: 65-67
🤖 Prompt for AI Agents
In e2e/tests/api/jobs/job-flow-entities.spec.yaml at lines 57-59 and 65-67, the
database resource entries incorrectly use the *kindService anchor. Replace the
*kindService anchor with *kindDatabase for these database resource definitions
to correctly reflect their kind and purpose.
| async function nextJobs( | ||
| api: Client<paths, `${string}/${string}`>, | ||
| agentId: string, | ||
| expectedJobCount: number, | ||
| ): Promise<Job[]> { | ||
| let jobs: Job[] = []; | ||
| let attempts = 0; | ||
|
|
||
| while (jobs.length < expectedJobCount && attempts < 10) { | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
| const fetchResponse = await api.GET("/v1/job-agents/{agentId}/queue/next", { | ||
| params: { | ||
| path: { | ||
| agentId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fetchResponse.response.ok).toBe(true); | ||
|
|
||
| const nextJobs = fetchResponse.data; | ||
| expect(nextJobs?.jobs).toBeDefined(); | ||
|
|
||
| expect(Array.isArray(nextJobs?.jobs)).toBe(true); | ||
| jobs.push(...((nextJobs?.jobs as Job[]) || [])); | ||
| } | ||
|
|
||
| expect( | ||
| jobs.every((job) => job.jobAgentId === agentId), | ||
| "expected agentId", | ||
| ).toBe(true); | ||
| expect( | ||
| _.uniq(jobs.map((job) => job.id)).length == jobs.length, | ||
| "expected unique jobIds", | ||
| ).toBe(true); | ||
|
|
||
| expect(jobs.length, "job count").toBe(expectedJobCount); | ||
| return jobs; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the polling logic for better reliability.
The current implementation has a few areas that could be improved:
- The timeout (10 seconds) might be insufficient under heavy load
- The function continues polling even after getting the expected number of jobs
- No clear error message when timeout is reached
Consider this improved implementation:
async function nextJobs(
api: Client<paths, `${string}/${string}`>,
agentId: string,
expectedJobCount: number,
+ maxAttempts: number = 10,
+ delayMs: number = 1000,
): Promise<Job[]> {
let jobs: Job[] = [];
let attempts = 0;
- while (jobs.length < expectedJobCount && attempts < 10) {
- await new Promise((resolve) => setTimeout(resolve, 1000));
+ while (jobs.length < expectedJobCount && attempts < maxAttempts) {
+ if (attempts > 0) {
+ await new Promise((resolve) => setTimeout(resolve, delayMs));
+ }
+ attempts++;
+
const fetchResponse = await api.GET("/v1/job-agents/{agentId}/queue/next", {
params: {
path: {
agentId,
},
},
});
expect(fetchResponse.response.ok).toBe(true);
const nextJobs = fetchResponse.data;
expect(nextJobs?.jobs).toBeDefined();
expect(Array.isArray(nextJobs?.jobs)).toBe(true);
jobs.push(...((nextJobs?.jobs as Job[]) || []));
+
+ // Early exit if we have enough jobs
+ if (jobs.length >= expectedJobCount) {
+ break;
+ }
}
+ if (jobs.length < expectedJobCount) {
+ throw new Error(`Timeout: Expected ${expectedJobCount} jobs but got ${jobs.length} after ${attempts} attempts`);
+ }
+
expect(
jobs.every((job) => job.jobAgentId === agentId),
"expected agentId",
).toBe(true);
expect(
_.uniq(jobs.map((job) => job.id)).length == jobs.length,
"expected unique jobIds",
).toBe(true);
expect(jobs.length, "job count").toBe(expectedJobCount);
return jobs;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function nextJobs( | |
| api: Client<paths, `${string}/${string}`>, | |
| agentId: string, | |
| expectedJobCount: number, | |
| ): Promise<Job[]> { | |
| let jobs: Job[] = []; | |
| let attempts = 0; | |
| while (jobs.length < expectedJobCount && attempts < 10) { | |
| await new Promise((resolve) => setTimeout(resolve, 1000)); | |
| const fetchResponse = await api.GET("/v1/job-agents/{agentId}/queue/next", { | |
| params: { | |
| path: { | |
| agentId, | |
| }, | |
| }, | |
| }); | |
| expect(fetchResponse.response.ok).toBe(true); | |
| const nextJobs = fetchResponse.data; | |
| expect(nextJobs?.jobs).toBeDefined(); | |
| expect(Array.isArray(nextJobs?.jobs)).toBe(true); | |
| jobs.push(...((nextJobs?.jobs as Job[]) || [])); | |
| } | |
| expect( | |
| jobs.every((job) => job.jobAgentId === agentId), | |
| "expected agentId", | |
| ).toBe(true); | |
| expect( | |
| _.uniq(jobs.map((job) => job.id)).length == jobs.length, | |
| "expected unique jobIds", | |
| ).toBe(true); | |
| expect(jobs.length, "job count").toBe(expectedJobCount); | |
| return jobs; | |
| } | |
| async function nextJobs( | |
| api: Client<paths, `${string}/${string}`>, | |
| agentId: string, | |
| expectedJobCount: number, | |
| maxAttempts: number = 10, | |
| delayMs: number = 1000, | |
| ): Promise<Job[]> { | |
| let jobs: Job[] = []; | |
| let attempts = 0; | |
| while (jobs.length < expectedJobCount && attempts < maxAttempts) { | |
| if (attempts > 0) { | |
| await new Promise((resolve) => setTimeout(resolve, delayMs)); | |
| } | |
| attempts++; | |
| const fetchResponse = await api.GET("/v1/job-agents/{agentId}/queue/next", { | |
| params: { | |
| path: { | |
| agentId, | |
| }, | |
| }, | |
| }); | |
| expect(fetchResponse.response.ok).toBe(true); | |
| const nextJobs = fetchResponse.data; | |
| expect(nextJobs?.jobs).toBeDefined(); | |
| expect(Array.isArray(nextJobs?.jobs)).toBe(true); | |
| jobs.push(...((nextJobs?.jobs as Job[]) || [])); | |
| // Early exit if we have enough jobs | |
| if (jobs.length >= expectedJobCount) { | |
| break; | |
| } | |
| } | |
| if (jobs.length < expectedJobCount) { | |
| throw new Error( | |
| `Timeout: Expected ${expectedJobCount} jobs but got ${jobs.length} after ${attempts} attempts` | |
| ); | |
| } | |
| expect( | |
| jobs.every((job) => job.jobAgentId === agentId), | |
| "expected agentId", | |
| ).toBe(true); | |
| expect( | |
| _.uniq(jobs.map((job) => job.id)).length == jobs.length, | |
| "expected unique jobIds", | |
| ).toBe(true); | |
| expect(jobs.length, "job count").toBe(expectedJobCount); | |
| return jobs; | |
| } |
🤖 Prompt for AI Agents
In e2e/tests/api/jobs/job-flow-1.spec.ts around lines 104 to 142, improve the
polling logic by adding a break condition to stop polling once the expected
number of jobs is reached, extend the timeout duration to handle heavy load
scenarios more reliably, and throw a clear error message if the timeout is
reached without fetching enough jobs. This will make the function more efficient
and provide better feedback on failure.
| public onePolicy(): PolicyRef { | ||
| if (this.policies.length === 0) { | ||
| throw new Error("No policies available."); | ||
| } | ||
| return this.policies[0]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make error handling consistent across all one* methods.
The onePolicy method explicitly checks for empty array and throws an error, while other one* methods rely on takeRandom to throw. This inconsistency could be confusing.
Either remove the explicit check in onePolicy to match other methods:
public onePolicy(): PolicyRef {
- if (this.policies.length === 0) {
- throw new Error("No policies available.");
- }
- return this.policies[0];
+ return takeRandom(this.policies, 1)[0];
}Or add similar checks to all one* methods for consistency. The current implementation would work fine with just takeRandom since it already handles the empty array case.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public onePolicy(): PolicyRef { | |
| if (this.policies.length === 0) { | |
| throw new Error("No policies available."); | |
| } | |
| return this.policies[0]; | |
| } | |
| public onePolicy(): PolicyRef { | |
| return takeRandom(this.policies, 1)[0]; | |
| } |
🤖 Prompt for AI Agents
In e2e/api/entity-refs.ts around lines 136 to 141, the onePolicy method
explicitly checks for an empty policies array and throws an error, while other
one* methods rely on the takeRandom function to handle empty arrays. To maintain
consistency, remove the explicit empty check and error throw in onePolicy and
instead return the result of takeRandom(this.policies), allowing takeRandom to
handle empty arrays uniformly across all one* methods.
| const fetchResponse = await this.api.POST(uriPath, { | ||
| body: requestBody, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add response status validation for consistency.
const fetchResponse = await this.api.POST(uriPath, {
body: requestBody,
});
+if (fetchResponse.response.status !== 201) {
+ throw new Error(
+ `Failed to create deployment: ${JSON.stringify(fetchResponse.error)}`,
+ );
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchResponse = await this.api.POST(uriPath, { | |
| body: requestBody, | |
| }); | |
| const fetchResponse = await this.api.POST(uriPath, { | |
| body: requestBody, | |
| }); | |
| if (fetchResponse.response.status !== 201) { | |
| throw new Error( | |
| `Failed to create deployment: ${JSON.stringify(fetchResponse.error)}`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In e2e/api/entities-builder.ts around lines 166 to 168, the POST request
response is not validated for its status. Add a check after the POST call to
verify that the response status indicates success (e.g., status code 200 or in
the 2xx range). If the status is not successful, handle the error appropriately,
such as throwing an error or logging a failure, to ensure consistent response
validation.
| const fetchResponse = await this.api.POST( | ||
| "/v1/deployments/{deploymentId}/variables", | ||
| { | ||
| params: { path: { deploymentId: deploymentResult.id } }, | ||
| body: { | ||
| ...variable, | ||
| directValues: variable.directValues?.map((dv) => ({ | ||
| ...dv, | ||
| resourceSelector: dv.resourceSelector ?? null, | ||
| })), | ||
| referenceValues: variable.referenceValues?.map((rv) => ({ | ||
| ...rv, | ||
| resourceSelector: rv.resourceSelector ?? null, | ||
| })), | ||
| requestBody, | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Fix incorrect request body wrapping.
The request body is incorrectly wrapped in an extra object layer. The requestBody variable already contains the body data.
const fetchResponse = await this.api.POST(
"/v1/deployments/{deploymentId}/variables",
{
params: { path: { deploymentId: deploymentResult.id } },
- body: {
- requestBody,
- },
+ body: requestBody,
},
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchResponse = await this.api.POST( | |
| "/v1/deployments/{deploymentId}/variables", | |
| { | |
| params: { path: { deploymentId: deploymentResult.id } }, | |
| body: { | |
| ...variable, | |
| directValues: variable.directValues?.map((dv) => ({ | |
| ...dv, | |
| resourceSelector: dv.resourceSelector ?? null, | |
| })), | |
| referenceValues: variable.referenceValues?.map((rv) => ({ | |
| ...rv, | |
| resourceSelector: rv.resourceSelector ?? null, | |
| })), | |
| requestBody, | |
| }, | |
| }, | |
| ); | |
| const fetchResponse = await this.api.POST( | |
| "/v1/deployments/{deploymentId}/variables", | |
| { | |
| params: { path: { deploymentId: deploymentResult.id } }, | |
| - body: { | |
| - requestBody, | |
| - }, | |
| + body: requestBody, | |
| }, | |
| ); |
🤖 Prompt for AI Agents
In e2e/api/entities-builder.ts around lines 246 to 254, the request body is
incorrectly wrapped inside an extra object layer by using { requestBody }.
Remove this wrapping and pass requestBody directly as the body in the POST
request to ensure the correct data structure is sent.
| const fetchResponse = await this.api.POST("/v1/environments", { | ||
| body: requestBody, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add response status validation.
For consistency with upsertSystem and to ensure proper error handling, validate the response status.
const fetchResponse = await this.api.POST("/v1/environments", {
body: requestBody,
});
+if (fetchResponse.response.status !== 201) {
+ throw new Error(
+ `Failed to create environment: ${JSON.stringify(fetchResponse.error)}`,
+ );
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchResponse = await this.api.POST("/v1/environments", { | |
| body: requestBody, | |
| }); | |
| const fetchResponse = await this.api.POST("/v1/environments", { | |
| body: requestBody, | |
| }); | |
| if (fetchResponse.response.status !== 201) { | |
| throw new Error( | |
| `Failed to create environment: ${JSON.stringify(fetchResponse.error)}`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In e2e/api/entities-builder.ts around lines 135 to 138, the POST request to
"/v1/environments" lacks validation of the response status. Add a check after
the POST call to verify that the response status indicates success (e.g., status
code 200 or 201). If the status is not successful, throw an error or handle it
appropriately to ensure consistent error handling like in the upsertSystem
method.
| const fetchResponse = await this.api.POST("/v1/resources", { | ||
| body: requestBody, | ||
| }); | ||
|
|
||
| if (resourceResponse.response.status !== 200) { | ||
| throw new Error( | ||
| `Failed to create resource: ${JSON.stringify( | ||
| resourceResponse.error, | ||
| )}`, | ||
| ); | ||
| } | ||
| results.push({ | ||
| fetchResponse, | ||
| requestBody, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add response status validation.
The method doesn't validate the response status, which could hide API failures.
const fetchResponse = await this.api.POST("/v1/resources", {
body: requestBody,
});
+if (fetchResponse.response.status !== 201) {
+ throw new Error(
+ `Failed to create resource: ${JSON.stringify(fetchResponse.error)}`,
+ );
+}
results.push({
fetchResponse,
requestBody,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchResponse = await this.api.POST("/v1/resources", { | |
| body: requestBody, | |
| }); | |
| if (resourceResponse.response.status !== 200) { | |
| throw new Error( | |
| `Failed to create resource: ${JSON.stringify( | |
| resourceResponse.error, | |
| )}`, | |
| ); | |
| } | |
| results.push({ | |
| fetchResponse, | |
| requestBody, | |
| }); | |
| } | |
| const fetchResponse = await this.api.POST("/v1/resources", { | |
| body: requestBody, | |
| }); | |
| if (fetchResponse.response.status !== 201) { | |
| throw new Error( | |
| `Failed to create resource: ${JSON.stringify(fetchResponse.error)}`, | |
| ); | |
| } | |
| results.push({ | |
| fetchResponse, | |
| requestBody, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In e2e/api/entities-builder.ts between lines 88 and 96, the code pushes the API
response to results without checking if the response status indicates success.
Add validation after the POST request to check the response status code, and
handle or throw an error if the status indicates failure, ensuring API failures
are detected and not silently ignored.
| for (const r of this.fixtures.resources) { | ||
| this.cache.resources.push({ | ||
| this.refs.resources.push({ | ||
| ...r, | ||
| originalIdentifier: r.identifier, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Use response data instead of fixture data to update refs.
The current implementation uses fixture data to update refs, which might miss server-generated fields like IDs or timestamps. This is inconsistent with other methods that correctly use response data.
-// Store created resources in result
-for (const r of this.fixtures.resources) {
- this.refs.resources.push({
- ...r,
- originalIdentifier: r.identifier,
- });
-}
+// Store created resources from responses
+for (let i = 0; i < results.length; i++) {
+ const response = results[i].fetchResponse;
+ const fixture = this.fixtures.resources[i];
+ if (response.data) {
+ this.refs.resources.push({
+ id: response.data.id,
+ identifier: response.data.identifier || fixture.identifier,
+ name: response.data.name || fixture.name,
+ type: response.data.type || fixture.type,
+ config: response.data.config || fixture.config,
+ originalIdentifier: fixture.identifier,
+ });
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const r of this.fixtures.resources) { | |
| this.cache.resources.push({ | |
| this.refs.resources.push({ | |
| ...r, | |
| originalIdentifier: r.identifier, | |
| }); | |
| } | |
| // Store created resources from responses | |
| for (let i = 0; i < results.length; i++) { | |
| const response = results[i].fetchResponse; | |
| const fixture = this.fixtures.resources[i]; | |
| if (response.data) { | |
| this.refs.resources.push({ | |
| id: response.data.id, | |
| identifier: response.data.identifier || fixture.identifier, | |
| name: response.data.name || fixture.name, | |
| type: response.data.type || fixture.type, | |
| config: response.data.config || fixture.config, | |
| originalIdentifier: fixture.identifier, | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In e2e/api/entities-builder.ts around lines 99 to 104, the code updates refs
using fixture data instead of the actual response data, which can omit
server-generated fields like IDs or timestamps. Modify the loop to iterate over
the response data received from the server rather than the fixture data, and
update this.refs.resources with the properties from the response objects to
ensure all server-generated fields are included.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation