test: split attachments.test.js#492
Conversation
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR splits a large attachments.test.js into focused files, which is a good structural improvement. Several correctness issues were found: silent test assertions inside .catch() blocks that never execute on 2xx responses (making tests always pass), a double-await pattern copied across two files, a wrong EICAR-like mime type in an error message assertion, a misleading comment that contradicts its assertion, and a magic -1 sentinel value in the shared utility function that could produce a malformed Content-Length header if misused.
PR Bot Information
Version: 1.29.2
- LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
328db820-8620-11f1-95b2-b3e78fa5cb02 - Experiment Variant:
control - Event Trigger:
pull_request.opened - File Content Strategy: Full file content
| const incidentID = await newIncident(POST, "processor") | ||
| let sampleDocID | ||
|
|
||
| const scanCleanWaiter = waitForScanStatus("Clean") |
There was a problem hiding this comment.
Logic Error: scanCleanWaiter is created before the attachment is uploaded, but the test never awaits it
scanCleanWaiter is declared and the waiter promise is created at line 351 before the upload, but the test never calls await scanCleanWaiter. Without this, the test proceeds to read content without waiting for the scan to complete. If the scan hasn't finished by the time the content GET happens, the test may fail non-deterministically or skip verifying the scan-clean state.
Consider adding await scanCleanWaiter after the GET at line 360 or before the content read, consistent with how other tests in this file use the waiter.
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
| await GET( | ||
| `odata/v4/processor/Incidents(ID=${incidentID},IsActiveEntity=true)/attachments(up__ID=${incidentID},ID=${sampleDocID},IsActiveEntity=true)/content`, | ||
| ).catch((e) => { | ||
| expect(e.status).toEqual(202) | ||
| expect(e.response.data.error.message).toContain( | ||
| "The previous scan was more than 3 days ago. Please try to download again in a moment, after the attachment is rescanned.", | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Logic Error: GET on a 202 response silently passes — test assertion is only inside .catch()
The .catch() block at line 277 expects e.status === 202, but a 202 is a successful HTTP status. The GET call will resolve (not reject) with status 202, so the .catch() callback is never invoked and the assertions are silently skipped. The test always passes regardless of what the server returns.
Consider using await GET(...).then(res => { expect(res.status).toEqual(202); ... }) or checking if axios throws for 2xx responses, or wrapping with a flag to ensure the assertion was reached:
const rescanRes = await GET(
`odata/v4/processor/Incidents(ID=${incidentID},IsActiveEntity=true)/attachments(up__ID=${incidentID},ID=${sampleDocID},IsActiveEntity=true)/content`,
).catch((e) => e.response)
expect(rescanRes.status).toEqual(202)
expect(rescanRes.data.error.message).toContain(
"The previous scan was more than 3 days ago. Please try to download again in a moment, after the attachment is rescanned.",
)| await GET( | |
| `odata/v4/processor/Incidents(ID=${incidentID},IsActiveEntity=true)/attachments(up__ID=${incidentID},ID=${sampleDocID},IsActiveEntity=true)/content`, | |
| ).catch((e) => { | |
| expect(e.status).toEqual(202) | |
| expect(e.response.data.error.message).toContain( | |
| "The previous scan was more than 3 days ago. Please try to download again in a moment, after the attachment is rescanned.", | |
| ) | |
| }) | |
| const rescanRes = await GET( | |
| `odata/v4/processor/Incidents(ID=${incidentID},IsActiveEntity=true)/attachments(up__ID=${incidentID},ID=${sampleDocID},IsActiveEntity=true)/content`, | |
| ).catch((e) => e.response) | |
| expect(rescanRes.status).toEqual(202) | |
| expect(rescanRes.data.error.message).toContain( | |
| "The previous scan was more than 3 days ago. Please try to download again in a moment, after the attachment is rescanned.", | |
| ) |
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
| await GET( | ||
| `odata/v4/processor/Incidents(ID=${incidentID},IsActiveEntity=true)/attachments(up__ID=${incidentID},ID=${sampleDocID},IsActiveEntity=true)/content`, | ||
| ).catch((e) => { | ||
| expect(e.status).toEqual(404) | ||
| expect(e.response.data.error.message).toMatch(/Not Found/) | ||
| }) |
There was a problem hiding this comment.
Logic Error: GET on a 404 response is asserted only inside .catch(), but may silently pass
The .catch() at line 416 only runs if the request throws. If the server returns 404 as a resolved response (some HTTP clients don't throw on 4xx), the assertions at lines 417–418 are never reached and the test silently passes without any verification. The same pattern exists at lines 437–440.
Consider using a boolean flag to guarantee the error handler was called:
let didThrow = false
await GET(...).catch((e) => {
didThrow = true
expect(e.status).toEqual(404)
expect(e.response.data.error.message).toMatch(/Not Found/)
})
expect(didThrow).toBe(true)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
| up__ID: testID, | ||
| filename: "testfile.pdf", | ||
| mimeType: "application/pdf", | ||
| createdAt: new Date(), |
There was a problem hiding this comment.
Bug: await await is a redundant double-await
await await runWithUser(...) at line 830 double-awaits the same promise. The inner await resolves the promise returned by runWithUser, and the outer await then awaits a non-Promise value, making the second await a no-op. This is identical to the same pattern at line 904 in attachments-rename.test.js. While harmless at runtime, it's misleading and may indicate a copy-paste oversight.
Should be const newAtt = await runWithUser(alice, () => ...).
| createdAt: new Date(), | |
| const newAtt = await runWithUser(alice, () => | |
| AttachmentsSrv.copy( | |
| Attachments, | |
| { ID: sourceAttachmentID }, | |
| Attachments.drafts, | |
| { | |
| up__ID: targetIncidentID, | |
| DraftAdministrativeData_DraftUUID: | |
| targetDraft.DraftAdministrativeData_DraftUUID, | |
| }, | |
| ), | |
| ) |
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
| `odata/v4/processor/Incidents(ID=${incidentID},IsActiveEntity=false)/attachments`, | ||
| ) | ||
| //the data should have no attachments | ||
| expect(response.status).toEqual(200) |
There was a problem hiding this comment.
Logic Error: Comment says "data should have no attachments" but expects exactly one
The inline comment at line 632 says //the data should have no attachments, but the assertion on line 634 expects response.data.value.length to equal 1. The comment is misleading and inverts the actual intent of the test.
Should be updated to //the data should have one attachment.
| expect(response.status).toEqual(200) | |
| //the data should have one attachment |
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
|
|
||
| //content should not be there | ||
| const responseContent = await GET( | ||
| `odata/v4/processor/Incidents(ID=${incidentID},IsActiveEntity=false)/attachments(up__ID=${incidentID},ID=${attachmentsID},IsActiveEntity=true)/content`, |
There was a problem hiding this comment.
Logic Error: The IsActiveEntity=true in the URL is wrong for a draft (false) context
At line 640, the GET request fetches attachment content using IsActiveEntity=true for what the test intends to be a draft-mode attachment (uploaded via INSERT.into(...drafts)). This likely causes the test to pass accidentally (or fail for the wrong reason) since the attachment was inserted into the draft table and would not yet be active. The path should use IsActiveEntity=false to match the draft-only attachment that was inserted.
| `odata/v4/processor/Incidents(ID=${incidentID},IsActiveEntity=false)/attachments(up__ID=${incidentID},ID=${attachmentsID},IsActiveEntity=true)/content`, | |
| `odata/v4/processor/Incidents(ID=${incidentID},IsActiveEntity=false)/attachments(up__ID=${incidentID},ID=${attachmentsID},IsActiveEntity=false)/content`, |
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
| try { | ||
| await waitForDeletion(attachmentResponse.data.value[0].url) | ||
| // Should throw due to timeout | ||
| expect(true).toEqual(false) |
There was a problem hiding this comment.
Logic Error: expect(true).toEqual(false) is not a meaningful assertion for "should have thrown"
At line 1938, the test intends to fail if waitForDeletion resolves without throwing. Using expect(true).toEqual(false) technically works but is opaque. A more idiomatic and readable approach is fail("waitForDeletion should have timed out but resolved instead") or throw new Error("Expected timeout but resolved"). Additionally, the comment at line 1937 ("Should throw due to timeout") should be phrased as "waitForDeletion should have rejected with a Timeout error" for clarity.
| expect(true).toEqual(false) | |
| throw new Error("Expected waitForDeletion to time out, but it resolved") |
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
| await POST( | ||
| `odata/v4/processor/Incidents(ID=${incidentID},IsActiveEntity=false)/mediaTypeAttachments`, | ||
| { | ||
| up__ID: incidentID, | ||
| filename: "sample.pdf", | ||
| mimeType: "application/jpeg charset=UTF-8", | ||
| createdAt: new Date( | ||
| Date.now() - Math.random() * 30 * 24 * 60 * 60 * 1000, | ||
| ), | ||
| createdBy: "alice", | ||
| }, | ||
| ).catch((e) => { | ||
| expect(e.status).toEqual(400) | ||
| expect(e.response.data.error.message).toMatch( | ||
| "The attachment file type 'application/pdf' is not allowed.", | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Logic Error: Wrong error message matched in .catch() — tests 'application/pdf' but mimeType sent is 'application/jpeg charset=UTF-8'
In the .catch() at lines 228–232, the expected error message checks for "The attachment file type 'application/pdf' is not allowed.", but the mimeType field sent in the POST body is "application/jpeg charset=UTF-8". The assertion will never match because the server would report the rejected mime type as application/jpeg, not application/pdf. This means any error thrown here (whether for the right or wrong reason) would fail the inner assertion, but because it's inside .catch() without a guarantee the callback runs, the test may silently pass.
The expected message should reference 'application/jpeg'.
| await POST( | |
| `odata/v4/processor/Incidents(ID=${incidentID},IsActiveEntity=false)/mediaTypeAttachments`, | |
| { | |
| up__ID: incidentID, | |
| filename: "sample.pdf", | |
| mimeType: "application/jpeg charset=UTF-8", | |
| createdAt: new Date( | |
| Date.now() - Math.random() * 30 * 24 * 60 * 60 * 1000, | |
| ), | |
| createdBy: "alice", | |
| }, | |
| ).catch((e) => { | |
| expect(e.status).toEqual(400) | |
| expect(e.response.data.error.message).toMatch( | |
| "The attachment file type 'application/pdf' is not allowed.", | |
| ) | |
| }) | |
| await POST( | |
| `odata/v4/processor/Incidents(ID=${incidentID},IsActiveEntity=false)/mediaTypeAttachments`, | |
| { | |
| up__ID: incidentID, | |
| filename: "sample.pdf", | |
| mimeType: "application/jpeg charset=UTF-8", | |
| createdAt: new Date( | |
| Date.now() - Math.random() * 30 * 24 * 60 * 60 * 1000, | |
| ), | |
| createdBy: "alice", | |
| }, | |
| ).catch((e) => { | |
| expect(e.status).toEqual(400) | |
| expect(e.response.data.error.message).toMatch( | |
| "The attachment file type 'application/jpeg' is not allowed.", | |
| ) | |
| }) |
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
| expect(targetDraft?.DraftAdministrativeData_DraftUUID).toBeTruthy() | ||
|
|
||
| // Source is the active Attachments entity (uploaded via draft, now active after save) | ||
| const newAtt = await await runWithUser(alice, () => |
There was a problem hiding this comment.
Bug: await await runWithUser(...) — redundant double-await
At line 904, const newAtt = await await runWithUser(...) double-awaits the same promise. The inner await already resolves the Promise returned by runWithUser; the outer await receives a plain value and is a no-op. Same issue exists in attachments-draft.test.js line 830.
Should be const newAtt = await runWithUser(alice, () => ...).
| const newAtt = await await runWithUser(alice, () => | |
| const newAtt = await runWithUser(alice, () => | |
| AttachmentsSrv.copy( | |
| Attachments, | |
| { ID: sourceAttachmentID }, | |
| Attachments.drafts, | |
| { | |
| up__ID: targetIncidentID, | |
| DraftAdministrativeData_DraftUUID: | |
| targetDraft.DraftAdministrativeData_DraftUUID, | |
| }, | |
| ), | |
| ) |
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
| overrideContentLength = -1, | ||
| entityName = "attachments", | ||
| ) { | ||
| const filepath = join(__dirname, "..", "integration", "content/sample.pdf") | ||
|
|
||
| await utils.draftModeEdit( | ||
| "processor", | ||
| "Incidents", | ||
| incidentId, | ||
| "ProcessorService", | ||
| ) | ||
|
|
||
| const res = await POST( | ||
| `odata/v4/processor/Incidents(ID=${incidentId},IsActiveEntity=false)/${entityName}`, | ||
| { | ||
| up__ID: incidentId, | ||
| filename: basename(filepath), | ||
| mimeType: "application/pdf", | ||
| createdAt: new Date( | ||
| Date.now() - Math.random() * 30 * 24 * 60 * 60 * 1000, | ||
| ), | ||
| createdBy: "alice", | ||
| }, | ||
| ) | ||
| const fileContent = readFileSync(filepath) | ||
| await PUT( | ||
| `/odata/v4/processor/Incidents_${entityName}(up__ID=${incidentId},ID=${res.data.ID},IsActiveEntity=false)/content`, | ||
| fileContent, | ||
| { | ||
| headers: { | ||
| "Content-Type": "application/pdf", | ||
| "Content-Length": | ||
| overrideContentLength != -1 |
There was a problem hiding this comment.
Logic Error: Sentinel value -1 used for "use default" is semantically unclear and fragile
The overrideContentLength parameter uses -1 as a sentinel to mean "use the actual file size". This makes the JSDoc misleading (-1 = use file size), requires callers to remember the magic value, and could silently produce an incorrect Content-Length: -1 header if a caller accidentally passes 0 instead of -1.
Consider using null or undefined as the sentinel value for "not overridden", which is more idiomatic in JavaScript:
async function uploadDraftAttachment(
utils, POST, PUT, GET, incidentId,
overrideContentLength = null,
entityName = "attachments",
) {
...
"Content-Length": overrideContentLength ?? fileContent.byteLength,| overrideContentLength = -1, | |
| entityName = "attachments", | |
| ) { | |
| const filepath = join(__dirname, "..", "integration", "content/sample.pdf") | |
| await utils.draftModeEdit( | |
| "processor", | |
| "Incidents", | |
| incidentId, | |
| "ProcessorService", | |
| ) | |
| const res = await POST( | |
| `odata/v4/processor/Incidents(ID=${incidentId},IsActiveEntity=false)/${entityName}`, | |
| { | |
| up__ID: incidentId, | |
| filename: basename(filepath), | |
| mimeType: "application/pdf", | |
| createdAt: new Date( | |
| Date.now() - Math.random() * 30 * 24 * 60 * 60 * 1000, | |
| ), | |
| createdBy: "alice", | |
| }, | |
| ) | |
| const fileContent = readFileSync(filepath) | |
| await PUT( | |
| `/odata/v4/processor/Incidents_${entityName}(up__ID=${incidentId},ID=${res.data.ID},IsActiveEntity=false)/content`, | |
| fileContent, | |
| { | |
| headers: { | |
| "Content-Type": "application/pdf", | |
| "Content-Length": | |
| overrideContentLength != -1 | |
| async function uploadDraftAttachment( | |
| utils, | |
| POST, | |
| PUT, | |
| GET, | |
| incidentId, | |
| overrideContentLength = null, | |
| entityName = "attachments", | |
| ) { | |
| const filepath = join(__dirname, "..", "integration", "content/sample.pdf") | |
| await utils.draftModeEdit( | |
| "processor", | |
| "Incidents", | |
| incidentId, | |
| "ProcessorService", | |
| ) | |
| const res = await POST( | |
| `odata/v4/processor/Incidents(ID=${incidentId},IsActiveEntity=false)/${entityName}`, | |
| { | |
| up__ID: incidentId, | |
| filename: basename(filepath), | |
| mimeType: "application/pdf", | |
| createdAt: new Date( | |
| Date.now() - Math.random() * 30 * 24 * 60 * 60 * 1000, | |
| ), | |
| createdBy: "alice", | |
| }, | |
| ) | |
| const fileContent = readFileSync(filepath) | |
| await PUT( | |
| `/odata/v4/processor/Incidents_${entityName}(up__ID=${incidentId},ID=${res.data.ID},IsActiveEntity=false)/content`, | |
| fileContent, | |
| { | |
| headers: { | |
| "Content-Type": "application/pdf", | |
| "Content-Length": overrideContentLength ?? fileContent.byteLength, | |
| }, | |
| }, | |
| ) |
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
1 participant
Uh oh!
There was an error while loading. Please reload this page.