Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

12 fix wrong response when repeated requests for external source #13

Conversation

b6pzeusbc54tvhw5jgpyw8pwz2x6gs
Copy link
Contributor

@ofhouse It's hard to set e2e:test envrionments. I ran docker container using docker-compose.yml, but many TCs still failed. So if you add guide for testing, I can run and add TCs.

@b6pzeusbc54tvhw5jgpyw8pwz2x6gs b6pzeusbc54tvhw5jgpyw8pwz2x6gs force-pushed the 12-fix-wrong-response-when-repeated-requests-for-external-source branch 2 times, most recently from f7dd653 to 92205f9 Compare March 1, 2021 18:36
@b6pzeusbc54tvhw5jgpyw8pwz2x6gs b6pzeusbc54tvhw5jgpyw8pwz2x6gs marked this pull request as draft March 1, 2021 18:59
@ofhouse
Copy link
Member

ofhouse commented Mar 1, 2021

Added a quick guide here for running e2e tests locally: https://github.com/dealmore/terraform-aws-next-js-image-optimization/blob/main/CONTRIBUTING.md

@ofhouse
Copy link
Member

ofhouse commented Mar 2, 2021

Whoops forgot to mention that also a local production build is required, updated the contribution guide accordingly.

@ofhouse
Copy link
Member

ofhouse commented Mar 8, 2021

@b6pzeusbc54tvhw5jgpyw8pwz2x6gs Anything here I can do to push this further?
If e2e don't work for you, I could offer to add them by myself.

@b6pzeusbc54tvhw5jgpyw8pwz2x6gs
Copy link
Contributor Author

I will try to add some TCs soon.

@b6pzeusbc54tvhw5jgpyw8pwz2x6gs b6pzeusbc54tvhw5jgpyw8pwz2x6gs force-pushed the 12-fix-wrong-response-when-repeated-requests-for-external-source branch 2 times, most recently from ae21226 to 94a48ef Compare March 11, 2021 18:36
@b6pzeusbc54tvhw5jgpyw8pwz2x6gs
Copy link
Contributor Author

The problem has not been reproduced by TC, so I am checking it. A little more modification is required.

@ofhouse
Copy link
Member

ofhouse commented Mar 12, 2021

Seems like the issue is hard to track down in a e2e test, because the stream finishes before the result is returned.
Maybe a unit test where the file read is delayed would make more sense?

@b6pzeusbc54tvhw5jgpyw8pwz2x6gs
Copy link
Contributor Author

b6pzeusbc54tvhw5jgpyw8pwz2x6gs commented Mar 13, 2021

Yes, it makes sense. Another reason is that the state is initialized every lambdaSAM.sendApiGwRequest call even the filesystem in the SAM local environment.

REPORT RequestId: 47f0c1bf-4125-471f-abba-e9e35cb24156	Init Duration: 0.34 ms	Duration: 485.10 ms	Billed Duration: 500 ms	Memory Size: 1024 MB	Max Memory Used: 1024 MB
REPORT RequestId: fb02f7e2-c8d8-4716-9bd7-24476e97cd69	Init Duration: 0.18 ms	Duration: 385.11 ms	Billed Duration: 400 ms	Memory Size: 1024 MB	Max Memory Used: 1024 MB

You can see that Init Duration is displayed in the above continuous call log.
I'm looking for a way to turn this initializing setting off.

@b6pzeusbc54tvhw5jgpyw8pwz2x6gs
Copy link
Contributor Author

b6pzeusbc54tvhw5jgpyw8pwz2x6gs commented Mar 13, 2021

  const apiSpawnArgs = [
      'local', 'start-api',
      '--port', `${port}`,
      '--warm-containers', 'EAGER',
    ];

@ofhouse I have successfully reproduced by modifying the createSAMLocal function to above on the sammy side.

Below is test result at e54c365.

 FAIL  test/e2e.test.ts (8.749 s)
  [e2e]
    ○ skipped Run test against domain that is not on the list
    Without S3
      ○ skipped External: Accept */*: bmp/test.bmp
      ○ skipped External: Accept */*: gif/test.gif
      ○ skipped External: Accept */*: jpeg/test.jpg
      ○ skipped External: Accept */*: png/test.png
      ○ skipped External: Accept */*: svg/test.svg
      ○ skipped External: Accept */*: tiff/test.tiff
      ○ skipped External: Accept */*: webp/test.webp
      ○ skipped External: Accept */*: webp/animated.webp
      ○ skipped Internal: Accept */*: bmp/test.bmp
      ○ skipped Internal: Accept */*: gif/test.gif
      ○ skipped Internal: Accept */*: jpeg/test.jpg
      ○ skipped Internal: Accept */*: png/test.png
      ○ skipped Internal: Accept */*: svg/test.svg
      ○ skipped Internal: Accept */*: tiff/test.tiff
      ○ skipped Internal: Accept */*: webp/test.webp
      ○ skipped Internal: Accept */*: webp/animated.webp
    With S3
      ○ skipped Internal: Fetch image from S3
    Filesystem cache
      ✓ Fetch external image by internet. (first call) (418 ms)
      ✕ Fetch external image by hitting filesystem cache. (2nd call) (32 ms)

  ● [e2e] › Filesystem cache › Fetch external image by hitting filesystem cache. (2nd call)

    Received content doesn't match the file external_accept_all_w-2048_q-75_png_test.png.png.

      280 |
      281 |       expect(response.status).toBe(200);
    > 282 |       expect(body).toMatchFile(snapshotFileName);
          |                    ^
      283 |       expect(response.headers.get('Content-Type')).toBe(
      284 |         fixtureResponse['content-type']
      285 |       );

      at test/e2e.test.ts:282:20
          at runMicrotasks (<anonymous>)

 › 1 snapshot failed.
Snapshot Summary
 › 1 snapshot failed from 1 test suite. Inspect your code changes or run `yarn run test:e2e -u` to update them.

Test Suites: 1 failed, 1 total
Tests:       1 failed, 18 skipped, 1 passed, 20 total
Snapshots:   1 failed, 1 total
Time:        8.782 s, estimated 9 s
Ran all test suites matching /e2e.*/i.
  console.log
    START RequestId: 135ccc57-61f2-4783-8beb-fe21f9facfa7 Version: $LATEST
    END RequestId: 135ccc57-61f2-4783-8beb-fe21f9facfa7
    REPORT RequestId: 135ccc57-61f2-4783-8beb-fe21f9facfa7	Init Duration: 0.23 ms	Duration: 381.64 ms	Billed Duration: 400 ms	Memory Size: 1024 MB	Max Memory Used: 1024 MB
    2021-03-13 19:42:09 127.0.0.1 - - [13/Mar/2021 19:42:09] "GET /_next/image?url=http%3A%2F%2F192.168.200.112%3A9000%2Fc884a8119848745b%2Fpng%2Ftest.png&w=2048&q=75 HTTP/1.1" 200 -

  console.log
    START RequestId: 386459dd-a858-4c0f-81fc-d408d8689f06 Version: $LATEST
    END RequestId: 386459dd-a858-4c0f-81fc-d408d8689f06
    REPORT RequestId: 386459dd-a858-4c0f-81fc-d408d8689f06	Duration: 4.12 ms	Billed Duration: 100 ms	Memory Size: 1024 MB	Max Memory Used: 1024 MB
    2021-03-13 19:42:09 127.0.0.1 - - [13/Mar/2021 19:42:09] "GET /_next/image?url=http%3A%2F%2F192.168.200.112%3A9000%2Fc884a8119848745b%2Fpng%2Ftest.png&w=2048&q=75 HTTP/1.1" 200 -

      at Object.onError (test/e2e.test.ts:249:36)

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Time: 0h:00m:10s

Can you add --warm-containers option to this? Please check milliHQ/sammy#1 first.

@b6pzeusbc54tvhw5jgpyw8pwz2x6gs b6pzeusbc54tvhw5jgpyw8pwz2x6gs force-pushed the 12-fix-wrong-response-when-repeated-requests-for-external-source branch 2 times, most recently from 7879748 to ec11689 Compare March 13, 2021 11:26
@b6pzeusbc54tvhw5jgpyw8pwz2x6gs b6pzeusbc54tvhw5jgpyw8pwz2x6gs marked this pull request as ready for review March 13, 2021 11:27
@ofhouse
Copy link
Member

ofhouse commented Mar 13, 2021

Thanks, added the warmup flag to sammy.
However when I run the test-file from your branch against the current main branch with sammy 1.4.0 the test is also passing, so it seems that it is not replicating the failing behaviour. 🙈

But tracking down this issue further would take too much time, so I would say the PR is fine with the test.

@b6pzeusbc54tvhw5jgpyw8pwz2x6gs
Copy link
Contributor Author

@ofhouse Sorry! There is a problem with that PR. Exported function generateSAM should be modified to pass cli options. I will create a new PR on Sammy.

@b6pzeusbc54tvhw5jgpyw8pwz2x6gs
Copy link
Contributor Author

Please check milliHQ/sammy#2 before mering this PR.

@b6pzeusbc54tvhw5jgpyw8pwz2x6gs b6pzeusbc54tvhw5jgpyw8pwz2x6gs force-pushed the 12-fix-wrong-response-when-repeated-requests-for-external-source branch 2 times, most recently from f836be6 to 647fb7c Compare March 14, 2021 17:46
- Add warmContainers, but it should wait for merging milliHQ/sammy#1 .
  So @ts-expect-error comment is added temporally.
@b6pzeusbc54tvhw5jgpyw8pwz2x6gs b6pzeusbc54tvhw5jgpyw8pwz2x6gs force-pushed the 12-fix-wrong-response-when-repeated-requests-for-external-source branch from 647fb7c to f302b0f Compare March 14, 2021 17:48
@b6pzeusbc54tvhw5jgpyw8pwz2x6gs
Copy link
Contributor Author

I just some "force push" to rebase latest main branch and remove unnecessary commit history. So, Be careful when checking out a branch.

@b6pzeusbc54tvhw5jgpyw8pwz2x6gs b6pzeusbc54tvhw5jgpyw8pwz2x6gs force-pushed the 12-fix-wrong-response-when-repeated-requests-for-external-source branch from f302b0f to 91e3596 Compare March 15, 2021 14:26
@b6pzeusbc54tvhw5jgpyw8pwz2x6gs
Copy link
Contributor Author

@ofhouse With sammy v1.5.0, I confirmed that both reproducing and resolving problem are working fine. Please check it.

Copy link
Member

@ofhouse ofhouse left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for taking the time and fixing the test! 👍
Just one thing that was left behind, then we can merge it!

lib/package.json Outdated Show resolved Hide resolved
@ofhouse ofhouse added this to the v10.0.5 milestone Mar 15, 2021
@ofhouse ofhouse self-requested a review March 15, 2021 18:17
@ofhouse ofhouse merged commit 9cfae50 into milliHQ:main Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

External source images are not responded from the repeated requests
2 participants