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

chore: wrap git api in try catch #595

Closed
wants to merge 5 commits into from

Conversation

sayali10
Copy link
Contributor

@sayali10 sayali10 commented Nov 20, 2024

📝 Description

We want to trigger comparadise based on the event trigger in our application instead of git PR.
For that we will be using unique id from app instead of commit hash to store new images.

While trying to run comparadise using unique id instead of commit hash, I am getting this error because the commit hash is invalid:

/home/runners/actions-runner/_work/_actions/ExpediaGroup/comparadise/v1/action/dist/main.js:26313
      const error = new RequestError(toErrorMessage(data), status, {
                    ^
RequestError [HttpError]: Not Found - https://docs.github.com/rest/commits/statuses#list-commit-statuses-for-a-reference
    at /home/runners/actions-runner/_work/_actions/ExpediaGroup/comparadise/v1/action/dist/main.js:26313:21
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async getLatestVisualRegressionStatus (/home/runners/actions-runner/_work/_actions/ExpediaGroup/comparadise/v1/action/dist/main.js:36109:20)
    at async run (/home/runners/actions-runner/_work/_actions/ExpediaGroup/comparadise/v1/action/dist/main.js:36163:40) {
  status: 404,

This PR wraps that call in try catch and returns null on error.
This way we would be able to save the new/diff images using unique id from app instead of commit hash.

@@ -42,8 +38,6 @@ export const run = async () => {
code => code !== 0
).length;

const latestVisualRegressionStatus =
await getLatestVisualRegressionStatus(commitHash);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it down just before the usage.

commitHash,
'failure',
VISUAL_TESTS_FAILED_TO_EXECUTE
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrapped it in common method so that we can print the error message => Failed to update commit status.
This would give little more clarity on reason why job failed.

'Failed to get latest visual regression status.'
);
warning(error as Error);
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using optional chaining operator wherever we are using latestVisualRegressionStatus value so returning null shouldn't break anything.

@danadajian
Copy link
Contributor

As discussed, we are going to add a new "diff id" param that can be used in place of commit hash

@danadajian danadajian closed this Nov 21, 2024
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.

2 participants