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: support coverage reports on external PRs #2087

Merged
merged 8 commits into from
Sep 27, 2022

Conversation

gastonfournier
Copy link
Contributor

About the changes

External PRs fail when trying to upload coverage reports (example). As explained in this documentation https://github.com/ArtiomTr/jest-coverage-report-action#outputs

this approach doesn't work with pull requests from forks without write permission

which is our case.

With this change, external PRs should be able to post the coverage report as a comment in the PR

@vercel
Copy link

vercel bot commented Sep 26, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
unleash-docs ✅ Ready (Inspect) Visit Preview Sep 27, 2022 at 1:52PM (UTC)
2 Ignored Deployments
Name Status Preview Updated
unleash ⬜️ Ignored (Inspect) Sep 27, 2022 at 1:52PM (UTC)
unleash-monorepo-frontend ⬜️ Ignored (Inspect) Sep 27, 2022 at 1:52PM (UTC)

@github-actions
Copy link

github-actions bot commented Sep 26, 2022

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
88.34% (-3.04% 🔻)
7069/8002
🟡 Branches
78.7% (-0.85% 🔻)
1101/1399
🟢 Functions
81.96% (-4.27% 🔻)
1990/2428
🟢 Lines
88.64% (-2.66% 🔻)
6554/7394

⚠️ Details were not displayed: the report size has exceeded the limit.

Test suite run success

1165 tests passing in 193 suites.

Report generated by 🧪jest coverage report action from b731e5b

@github-actions
Copy link

github-actions bot commented Sep 26, 2022

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
88.58% (-2.79% 🔻)
7098/8013
🟡 Branches
78.91% (-0.64% 🔻)
1104/1399
🟢 Functions
82.34% (-3.89% 🔻)
2000/2429
🟢 Lines
88.9% (-2.4% 🔻)
6583/7405

⚠️ Details were not displayed: the report size has exceeded the limit.

Test suite run success

1170 tests passing in 194 suites.

Report generated by 🧪jest coverage report action from 280a941

Copy link
Contributor

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

Looks great :)

Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'm wondering whether secrets.GITHUB_TOKEN will work or not. The docs mention that the default uses github.token (whatever that is) and the example it has does not specify the token. Any reason why we need the secret token? If not, maybe we could try and remove it?

.github/workflows/build_prs_jest_report.yaml Outdated Show resolved Hide resolved
@gastonfournier
Copy link
Contributor Author

Here we managed to reproduce the issue: #2089 (comment) because I've opened this from an external repository

Gastón Fournier and others added 5 commits September 26, 2022 14:50
@gastonfournier
Copy link
Contributor Author

External PRs fail due to lack of permissions. It's still possible to keep the tests and coverage but skip the publishing (that now is split into another step)
image

Copy link
Contributor

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

Nice one! So what's the current state? Will the coverage report run on external PRs or not? Does it still fail? The action readme says:

Error: Resource not accessible by integration

This message means the requester does not have enough permission. If secrets.GITHUB_TOKEN is explicitly passed, this problem can be solved by just removing it.

But we're not passing it anyway 🤔 This issue also has a number of suggestions, which may or may not work and may or may not be worth it.

@gastonfournier
Copy link
Contributor Author

@thomasheartman we tried fixing this in multiple ways with @gardleopard using another repo (https://github.com/gastonfournier/test-pr-comment) to speed up the feedback loop and after some attempts, we're removing the comment. We can look into this later with more time

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

Successfully merging this pull request may close these issues.

None yet

4 participants