Skip to content

tests: robust external api handling #28

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SirSimon04 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

tests: robust external api handling #28

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 24 additions & 14 deletions tests/integration/mtx.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,18 @@ describe('AICore MTX resource group lifecycle', { concurrency: false, timeout: 3
for (let i = 0; i < 30; i++) {
await new Promise((r) => setTimeout(r, 2000)); // eslint-disable-line no-await-in-loop
cds.context = new cds.EventContext({ tenant: testTenantId });
// eslint-disable-next-line no-await-in-loop
const groups = await aiCore.run(
SELECT.from('AICore.resourceGroups').where({ tenantId: testTenantId })
);
if (Array.isArray(groups) && groups[0]?.status === 'PROVISIONED') {
provisioned = true;
break;
try {
// eslint-disable-next-line no-await-in-loop
const groups = await aiCore.run(
SELECT.from('AICore.resourceGroups').where({ tenantId: testTenantId })
);
if (Array.isArray(groups) && groups[0]?.status === 'PROVISIONED') {
provisioned = true;
break;
}
} catch (e) {
// Transient network error — continue polling
console.warn(`Provisioning poll attempt ${i + 1} failed: ${e.message}`);
Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: e.message may be undefined if the caught error is not an Error instance

If aiCore.run(...) rejects with a non-Error value (e.g., a plain string or an object), e.message will be undefined, producing a misleading log line like "Provisioning poll attempt 1 failed: undefined".

Consider using String(e) or e?.message ?? e to safely stringify any rejection value.

Suggested change
console.warn(`Provisioning poll attempt ${i + 1} failed: ${e.message}`);
console.warn(`Provisioning poll attempt ${i + 1} failed: ${e?.message ?? e}`);

Double-check suggestion before committing. Edit this comment for amendments.

Please provide feedback on the review comment by checking the appropriate box:

  • 🌟 Awesome comment, a human might have missed that.
  • ✅ Helpful comment
  • 🤷 Neutral
  • ❌ This comment is not helpful

Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic Error: The first subscribe polling loop (lines 55–63) is left without the same error-handling treatment

The two polling loops added in this PR are now wrapped in try/catch to tolerate transient network errors, but the earlier poll in the subscribe creates a resource group test (lines 59–61) still throws unhandled, causing the entire test to fail on any transient error. For consistency and robustness, that loop should be wrapped as well.

Please provide feedback on the review comment by checking the appropriate box:

  • 🌟 Awesome comment, a human might have missed that.
  • ✅ Helpful comment
  • 🤷 Neutral
  • ❌ This comment is not helpful

}
}
assert.ok(provisioned, 'Resource group should reach PROVISIONED before unsubscribe');
Expand All @@ -96,13 +101,18 @@ describe('AICore MTX resource group lifecycle', { concurrency: false, timeout: 3
for (let i = 0; i < 36; i++) {
await new Promise((r) => setTimeout(r, 5000)); // eslint-disable-line no-await-in-loop
cds.context = new cds.EventContext({ tenant: testTenantId });
// eslint-disable-next-line no-await-in-loop
const groups = await aiCore.run(
SELECT.from('AICore.resourceGroups').where({ tenantId: testTenantId })
);
if (!Array.isArray(groups) || groups.length === 0) {
deleted = true;
break;
try {
// eslint-disable-next-line no-await-in-loop
const groups = await aiCore.run(
SELECT.from('AICore.resourceGroups').where({ tenantId: testTenantId })
);
if (!Array.isArray(groups) || groups.length === 0) {
deleted = true;
break;
}
} catch (e) {
// Transient network error — continue polling
console.warn(`Deletion poll attempt ${i + 1} failed: ${e.message}`);
Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot May 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: e.message may be undefined if the caught error is not an Error instance

Same issue as in the provisioning loop above: if the rejection value is a plain string or object, e.message will be undefined.

Consider using e?.message ?? e for a safe fallback.

Suggested change
console.warn(`Deletion poll attempt ${i + 1} failed: ${e.message}`);
console.warn(`Deletion poll attempt ${i + 1} failed: ${e?.message ?? e}`);

Double-check suggestion before committing. Edit this comment for amendments.

Please provide feedback on the review comment by checking the appropriate box:

  • 🌟 Awesome comment, a human might have missed that.
  • ✅ Helpful comment
  • 🤷 Neutral
  • ❌ This comment is not helpful

}
}
assert.strictEqual(
Expand Down
Loading