Conversation
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR contains two meaningful changes: a genuine correctness fix (await cds.tx(...) ensuring the "Scanning" status is committed before the scan is triggered) and the removal of a duplicate/misplaced cds.spawn block. The main concern is the afterAll(() => delay(2000)) teardown strategy added to two test files — it replaces race conditions with a hardcoded sleep, which is fragile and should be replaced with deterministic event-based waiting using the existing waitForScanStatus utility.
PR Bot Information
Version: 1.20.0 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- LLM:
anthropic--claude-4.6-sonnet - File Content Strategy: Full file content
- Correlation ID:
7feae410-2e8e-11f1-8442-1562ace104f5 - Event Trigger:
pull_request.opened
| const { createAttachmentMetadata, uploadAttachmentContent } = createHelpers() | ||
|
|
||
| // Allow background operations (malware scan status updates) to complete before teardown | ||
| afterAll(() => delay(2000)) |
There was a problem hiding this comment.
Best Practices: Hardcoded delay(2000) is a fragile teardown strategy
Using a fixed 2-second sleep to wait for background operations (malware scan status updates) is non-deterministic: it will silently pass on fast machines and silently fail on slow ones or under load. The codebase already has a purpose-built waitForScanStatus utility that properly awaits the actual completion event with a timeout.
Consider replacing the blind delay with a proper awaitable hook that tracks pending background operations, or at minimum document what specific operations are expected to complete and the rationale for the chosen timeout value. If this test suite consistently triggers a scan, waitForScanStatus from testUtils should be used instead.
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
| axios.defaults.auth = { username: "alice" } | ||
|
|
||
| // Allow background operations (malware scan status updates) to complete before teardown | ||
| afterAll(() => delay(2000)) |
There was a problem hiding this comment.
Best Practices: Hardcoded delay(2000) is a fragile teardown strategy
Same issue as in the integration test: a fixed 2-second sleep is non-deterministic and could either waste time or still race on heavily loaded CI runners. If the background operations being awaited are detectable events (e.g., AttachmentUploadRejected emissions from cds.spawn), the teardown should await those events explicitly rather than sleeping for an arbitrary duration.
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
1 participant
Fix: Improve Attachment Validation and Testing Stability with Delay
Bug Fix
🐛 Addresses two issues: a missing
awaiton acds.tx()call in attachment scan expiry handling, and test instability caused by background operations (malware scan status updates) not completing before test teardown.Additionally, a redundant
cds.spawnblock for emittingAttachmentSizeExceededevents was removed, as the event emission was already handled in an earlier block.Changes
lib/generic-handlers.js:awaitbeforecds.tx()when updating attachment scan status to"Scanning", ensuring the transaction completes before proceeding.cds.spawnblock that was emittingAttachmentSizeExceededevents (the event is already emitted in a prior code block).tests/integration/attachments-non-draft.test.js:delayutility fromtestUtils.afterAll(() => delay(2000))hook to allow background operations (malware scan status updates) to finish before test teardown, improving test stability.tests/unit/validateAttachmentMimeType.test.js:delayutility fromtestUtils.afterAll(() => delay(2000))hook for the same reason — preventing flaky test failures caused by incomplete background operations at teardown.📬 Subscribe to the Hyperspace PR Bot DL to get the latest announcements and pilot features!
PR Bot Information
Version:
1.20.0| 📖 Documentation | 🚨 Create Incident | 💬 Feedbackanthropic--claude-4.6-sonnet7feae410-2e8e-11f1-8442-1562ace104f5pull_request.opened