tests: robust external api handling#28
Conversation
There was a problem hiding this comment.
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}`); |
There was a problem hiding this comment.
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.
| 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}`); |
There was a problem hiding this comment.
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.
| 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}`); |
There was a problem hiding this comment.
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
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) intry/catchblocks to gracefully handle transient network errors when queryingAICore.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.PR Bot Information
Version:
1.20.4765922c6a-0cf0-4f78-89cf-91d0f4416e4bpull_request.openedanthropic--claude-4.6-sonnet