Skip to content

tests: robust external api handling#28

Open
SirSimon04 wants to merge 1 commit into
mainfrom
tests-robust-api-calls
Open

tests: robust external api handling#28
SirSimon04 wants to merge 1 commit into
mainfrom
tests-robust-api-calls

Conversation

@SirSimon04
Copy link
Copy Markdown
Contributor

@SirSimon04 SirSimon04 commented May 13, 2026

Tests: Robust External API Handling in MTX Integration Tests

Test

🧪 Improved resilience of MTX integration tests by adding error handling around external API polling calls to prevent transient network errors from failing the test suite.

Changes

  • tests/integration/mtx.test.js: Wrapped both polling loops (provisioning and deletion checks) in try/catch blocks to gracefully handle transient network errors when querying AICore.resourceGroups. Instead of failing immediately on a network error, the polling continues to the next iteration and logs a warning message, making the tests more robust against intermittent API failures.
  • 🔄 Regenerate and Update Summary
PR Bot Information

Version: 1.20.47

  • Correlation ID: 65922c6a-0cf0-4f78-89cf-91d0f4416e4b
  • Event Trigger: pull_request.opened
  • LLM: anthropic--claude-4.6-sonnet
  • Summary Prompt: Default Prompt
  • Output Template: Default Template
  • File Content Strategy: Full file content

@SirSimon04 SirSimon04 requested a review from a team as a code owner May 13, 2026 07:54
Copy link
Copy Markdown
Contributor

@hyperspace-insights hyperspace-insights Bot left a comment

Choose a reason for hiding this comment

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

The PR's intent is sound — wrapping the polling calls in try/catch to absorb transient network errors is a good pattern — but the implementation has two minor robustness gaps: unsafe e.message access on non-Error rejections, and the first subscribe polling loop being left unprotected while the two new loops are now guarded.

PR Bot Information

Version: 1.20.47

  • File Content Strategy: Full file content
  • Event Trigger: pull_request.opened
  • LLM: anthropic--claude-4.6-sonnet
  • Correlation ID: 65922c6a-0cf0-4f78-89cf-91d0f4416e4b

}
} catch (e) {
// Transient network error — continue polling
console.warn(`Provisioning poll attempt ${i + 1} failed: ${e.message}`);
Copy link
Copy Markdown
Contributor

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

}
} catch (e) {
// Transient network error — continue polling
console.warn(`Deletion poll attempt ${i + 1} failed: ${e.message}`);
Copy link
Copy Markdown
Contributor

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

}
} catch (e) {
// Transient network error — continue polling
console.warn(`Provisioning poll attempt ${i + 1} failed: ${e.message}`);
Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant