CCM-12183 Component test for letter PDF #367
Conversation
| // Cap wait to avoid extremely long waits | ||
| const MAX_WAIT_MS = 120_000; // 2 minutes cap | ||
| const effectiveWait = Math.min(waitMs, MAX_WAIT_MS); | ||
| await new Promise((resolve) => { |
There was a problem hiding this comment.
Is there a reason why we always wait 2 minutes before we call fetchAndValidatePdf()?
Are we waiting for it to be available in AWS before we fetch it?
There was a problem hiding this comment.
The expiry of the download url is 1 min. So maybe not worth waiting that long if we want to test the expiry?
There was a problem hiding this comment.
Wait time is capped at 2mins so that tests don't go into long waits if X-Amz-Expires is >2mins
There was a problem hiding this comment.
If the X-Amz-Expires is bigger than 2 mins then I think the test would fail, as 2 mins (the cap) wouldn't be enough to trigger the 403?
| // const responseBody = await response.text(); | ||
| // expect(responseBody).toContain("PDF"); |
There was a problem hiding this comment.
Can be removed?
| // Cap wait to avoid extremely long waits | ||
| const MAX_WAIT_MS = 120_000; // 2 minutes cap | ||
| const effectiveWait = Math.min(waitMs, MAX_WAIT_MS); | ||
| await new Promise((resolve) => { |
There was a problem hiding this comment.
The expiry of the download url is 1 min. So maybe not worth waiting that long if we want to test the expiry?
| }; | ||
| } | ||
|
|
||
| const pdfUrl = await response.url(); |
There was a problem hiding this comment.
I wonder what this url really is and if it's the presigned URL... Maybe playwright gives it "for free" in the response. By default, playwright's get method follows redirects (https://playwright.dev/docs/api/class-apirequestcontext#api-request-context-get), so when the get letter data api returns 303, it will go automatically to the url in the Location header, and gets the pdf, hence why we see 200 being the response code. Don't think we need to change anything but maybe worth putting a comment so we know how it's working?
| const url = await fetchAndValidatePdf(pdfUrl); | ||
| expect(url.status).toBe(200); |
There was a problem hiding this comment.
This has already been tested above, because of the assertion on line 40 for the status = 200. So this is getting the pdf a second time.
| // const responseBody = await response.text(); | ||
| // expect(responseBody).toContain("PDF"); | ||
|
|
||
| async function fetchAndValidatePdf(url: string) { |
There was a problem hiding this comment.
It's called fetchAndValidatePdf but it's not validating anything I think?
| }); | ||
|
|
||
| const after = await fetchAndValidatePdf(pdfUrl); | ||
| expect([400, 403]).toContain(after.status); |
There was a problem hiding this comment.
I think we should narrow this to 403 as 400 could mean the test didn't go as planned. It should say it's forbidden (403) accessing that url, not that the request is malformed (400)
| expect(responseBody).toMatchObject(error404ResponseBody()); | ||
| }); | ||
|
|
||
| test(`Get /letters/{id}/data returns 500 if letter is not found for supplierId ${SUPPLIERID}`, async ({ |
There was a problem hiding this comment.
This should be 404. If it's 500 we've got a bug in the get letter data endpoint.
There was a problem hiding this comment.
It's because of the isNotFoundError in letter-operations, the \w+ isn't doing what's expected - "A word character is a character a-z, A-Z, 0-9, including _ (underscore).", so it doesn't match UUIDs for example.
const test = /^Letter with id \w+ not found for supplier \w+$/.test("Letter with id d91ed05e8 not found for supplier supplier1");
console.log(test);
> true
const test = /^Letter with id \w+ not found for supplier \w+$/.test("Letter with id d91e-d05e8 not found for supplier supplier1");
console.log(test);
> false
|
|
||
| const pdfUrl = await response.url(); | ||
| const parsed = new URL(pdfUrl); | ||
| const expiresParam = parsed.searchParams.get("X-Amz-Expires"); |
There was a problem hiding this comment.
Maybe worth asserting this should be 60 (seconds)?
5 participants
Description
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.