Skip to content

IOS-9864: Update record-screenshots to also pass the tests #342

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

Merged
merged 4 commits into from
Mar 8, 2024

Conversation

franciscojrp
Copy link
Member

@franciscojrp franciscojrp commented Mar 6, 2024

IOS-9864

🥅 What's the goal?

Update the GitHub action to record the screenshots so it also executes the tests.
Also change the action so it is executed by inserting /record-screenshots as comment on a PR to avoid branch mistakes and to be able to set the commit and the PR status after passing the tests.

🚧 How do we do it?

  • Execute the tests after recording the screenshots.
  • Launch the action by commenting on a PR.
  • Update the latest commit status and thus the PR status with the tests result.

Launch the action by commenting on a PR.
Execute the tests after recording the screenshots.
Update the latest commit status and thus the PR with the tests result.
Copy link

github-actions bot commented Mar 6, 2024

Screenshot tests report

✔️ All passing

@franciscojrp franciscojrp marked this pull request as ready for review March 6, 2024 15:32
@franciscojrp franciscojrp changed the title Update record-screenshots.yml Update record-screenshots to also pass the tests Mar 6, 2024
name: Execute tests
needs: record-screenshots
uses: ./.github/workflows/ci.yml
secrets: inherit
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@DavidMarinCalleja DavidMarinCalleja left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@salavert salavert left a comment

Choose a reason for hiding this comment

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

Nice

runs-on: self-hosted-novum-mac
steps:
- name: Get branch of PR
uses: xt0rted/pull-request-comment-branch@v2
Copy link
Contributor

@alexanegon alexanegon Mar 7, 2024

Choose a reason for hiding this comment

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

IMHO I would develop it ourselves instead of using third-party repositories

GitHub provides very useful variables, you'll need to process them a bit to get the values you desire:

GITHUB_SHA: The commit SHA that triggered the workflow.
GITHUB_REF: The branch or tag ref that triggered the workflow.

DOC

Or if you need to get the base branch of a PR you can just do: ${{ github.event.pull_request.base.ref }}

My concern is that these repositories don't seem to be very maintained. IMHO we could create our own actions in Telefonica/github-actions so everybody could use them

Copy link
Member Author

Choose a reason for hiding this comment

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

What you reference would not work because the action, when triggered from a issue_comment trigger, runs on the main branch instead of the PR branch.

In the repository of that action it's explained:

The pull request head ref and sha are important because issue_comment workflows run against the repository's default branch (usually master) and not the pull request's branch. With this action you'll be able to pass the ref to [actions/checkout](https://github.com/actions/checkout) and work with the pull request's code.

There is an open discussion in the actions repository about this, but the solutions seem more hacky to me and prone to failing without warning than using an action that works well: actions/checkout#331

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't done much research, if you think it's going to take a long time to do it on our own, okay, but if not, I think we should do it ourselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMHO It is good not being dependent on third party actions, but it is difficult and time consuming; we were already using one in this workflow and now two more, because it is not just this one you comment, we also need another one to set the commit status.

Copy link
Contributor

@alejandroruizponce alejandroruizponce left a comment

Choose a reason for hiding this comment

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

Great work!!

@alexanegon alexanegon self-requested a review March 7, 2024 10:15
@franciscojrp franciscojrp changed the title Update record-screenshots to also pass the tests IOS-9864: Update record-screenshots to also pass the tests Mar 8, 2024
@franciscojrp franciscojrp merged commit f70c3c9 into main Mar 8, 2024
@franciscojrp franciscojrp deleted the record-screenshots-also-pass-tests branch March 8, 2024 09:02
@tuentisre
Copy link
Collaborator

🎉 This PR is included in version 29.5.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.