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

test(adapters-e2e): deploy to platform instead of ntl serve #38643

Merged
merged 5 commits into from
Oct 19, 2023

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Oct 18, 2023

Description

This moves adapters e2e tests setup to actually deploy and test against platform and not against ntl serve.

Documentation

Tests

One test was adjusted a bit to pass (setup isn't great overall for this test, so while not ideal - this should do for now to get e2e tests to use platform and be able to start adding more tests along the adapter related fixes for more confidence overall)

Related Issues

FRA-44

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 18, 2023
@pieh pieh added topic: adapters Related to Gatsby Adapters and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Oct 18, 2023
Comment on lines -6 to +17
cy.intercept("/static/astro-**.png").as("img-import")
cy.intercept("/static/astro-**.png", req => {
req.on("before:response", res => {
// this generally should be permamently cached, but that cause problems with intercepting
// see https://docs.cypress.io/api/commands/intercept#cyintercept-and-request-caching
// so we disable caching for this response
// tests for cache-control headers should be done elsewhere

cy.visit('/').waitForRouteChange()
res.headers["cache-control"] = "no-store"
})
}).as("img-import")

cy.visit("/").waitForRouteChange()
Copy link
Contributor Author

@pieh pieh Oct 19, 2023

Choose a reason for hiding this comment

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

This is only real change in test - rest of it is just autoformatting (yikes). Apperently with ntl serve permanent caching wasn't really happing and test setup is kind of meh when that happens ( https://docs.cypress.io/api/commands/intercept#cyintercept-and-request-caching ), so this is just workaround to get test working (and probably note to actually add proper caching behavior tests in the future PR)

@pieh pieh marked this pull request as ready for review October 19, 2023 09:16
Copy link
Contributor

@JGAntunes JGAntunes left a comment

Choose a reason for hiding this comment

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

Overall LGTM, don’t really have any blockers but a couple of questions only:

  • Any particular reason not to integrate with Netlify’s CI/CD system instead and let these build on push as deploy previews, etc. instead of going through the CLI triggers?
  • Seems like the deletion of deploys is configurable but wondering if we might want to always keep those around for debug purposes?

@pieh
Copy link
Contributor Author

pieh commented Oct 19, 2023

  • Any particular reason not to integrate with Netlify’s CI/CD system instead and let these build on push as deploy previews, etc. instead of going through the CLI triggers?

This was easier to setup for build to use changes in Pull Requests (because it was already using it). To use Netlify's CI/CD I would need to come up with a method to build packages here and link them together (I think that would be doable, would just require extra effort)

  • Seems like the deletion of deploys is configurable but wondering if we might want to always keep those around for debug purposes?

For next-runtime we started deleting deploys because of https://github.com/netlify/pod-ecosystem-frameworks/issues/423 so I just wanted to get gatsby tests behave the same to avoid similar problems. The deploy deletion code is almost 1:1 copy from Next tests btw ( https://github.com/netlify/next-runtime/blob/2fca7b39eede8e25a305d0468d5fbeeb6000a02c/test/e2e/next-test-lib/next-modes/next-deploy.ts#L113-L126 )

@pieh pieh merged commit 5bc0992 into master Oct 19, 2023
32 checks passed
@pieh pieh deleted the e2e-adapter-deploy branch October 19, 2023 14:41
pieh added a commit that referenced this pull request Oct 24, 2023
* test(adapters-e2e): deploy to platform instead of ntl serve

* test: clear browser cache for interception tests

* test: delete deploy after the test

* test: how about not caching things to begin with?

* chore: update comment, only disable caching for webpack asset, restore previous assertions

(cherry picked from commit 5bc0992)
pieh added a commit that referenced this pull request Oct 25, 2023
…38662)

* test(adapters-e2e): deploy to platform instead of ntl serve

* test: clear browser cache for interception tests

* test: delete deploy after the test

* test: how about not caching things to begin with?

* chore: update comment, only disable caching for webpack asset, restore previous assertions

(cherry picked from commit 5bc0992)

Co-authored-by: Michal Piechowiak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: adapters Related to Gatsby Adapters
Projects
Status: Published
Development

Successfully merging this pull request may close these issues.

3 participants