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

fix: netlify-edge-functions-test for PRs and jest tests #3229

Merged
merged 5 commits into from
Sep 17, 2024

Conversation

anshgoyalevil
Copy link
Member

@anshgoyalevil anshgoyalevil commented Sep 13, 2024

Description

  • This PR basically fixes some things left in PR test: add tests for netlify functions #2493
  • This PR adds the target where the workflow should be triggered.
  • Also, a minor fix in the test statement is introduced to exclude jest tests from being executed by deno.

Copy link

netlify bot commented Sep 13, 2024

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit ea2f7de
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/66e853dbf5f0ff000859cf44
😎 Deploy Preview https://deploy-preview-3229--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Sep 13, 2024

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 43
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on https://deploy-preview-3229--asyncapi-website.netlify.app/

@@ -1,20 +1,21 @@
name: Run tests for netlify edge-functions

on:
workflow_dispatch
pull_request:
types: [opened, reopened, synchronize, ready_for_review]
Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to test netlify function this many times on each PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we calling in the actual netlify function while testing? Not sure, maybe @Shurtu-gal might help here.
Is that's not the case, then this is okay, since we do that number of times for jest tests also.

Copy link
Contributor

Choose a reason for hiding this comment

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

The actual netlify function is not called at all. Instead the logic is tested through mocking.

mf.install();
mf.mock("*", (req) => {
console.log(req.url);
if (req.url === metricURL) {
metricCalls++;
}
const body = {
url: req.url,
method: req.method,
headers: req.headers,
};
return new Response(JSON.stringify(body), {
status: 200,
headers: {
"Content-Type": "application/json",
},
});
});
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for clarification @Shurtu-gal
@akshatnema I think we can then go for running netlify function tests for every PR this number of times. Afterall, they are taking around 11 seconds

@akshatnema
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit 029a88b into asyncapi:master Sep 17, 2024
16 checks passed
@anshgoyalevil anshgoyalevil deleted the netlify-wf-update branch September 17, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants