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

Provide a solution to create/update reference screenshots #5

Merged
merged 9 commits into from
Jun 1, 2023

Conversation

raphiz
Copy link
Member

@raphiz raphiz commented May 4, 2023

No description provided.

@raphiz raphiz requested a review from mloydd May 4, 2023 07:37
@raphiz raphiz force-pushed the add-update-screenshots-functionallity branch from d14a124 to d285a73 Compare May 4, 2023 08:05
it("screenshot takes a screenshot of the given url", async () => {
await renderer.start();

const screenshot = await renderer.screenshot("https://ergon.ch");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test will fail if ergon.ch website is down.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm not 100% happy with this test case, but since the renderer accepts an URL, this was the easiest thing to get it to green. The safe way to do it would be to spin up a local server and use this url instead - however, this would make the test much more complex. I think for now, it's fine to ignore this, ok?

@@ -0,0 +1,4 @@
export function getLocalAddress(fastifyInstance) {
let address = fastifyInstance.addresses().find((e) => e.family === "IPv4");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let address = fastifyInstance.addresses().find((e) => e.family === "IPv4");
const address = fastifyInstance.addresses().find((e) => e.family === "IPv4");

Copy link
Collaborator

Choose a reason for hiding this comment

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

address can potentially be undefined. I think we need to check for the undefined case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will throw here an explicit exception if address is undefined

@raphiz raphiz force-pushed the add-update-screenshots-functionallity branch from 332e2e7 to b27896e Compare June 1, 2023 05:54
@raphiz raphiz changed the base branch from refactor-imported-code to main June 1, 2023 06:17
@raphiz raphiz merged commit 5db158f into main Jun 1, 2023
1 check passed
@raphiz raphiz deleted the add-update-screenshots-functionallity branch June 1, 2023 06:17
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.

None yet

2 participants