Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

ci: comment compare #1782

Merged
merged 9 commits into from
Nov 16, 2021
Merged

ci: comment compare #1782

merged 9 commits into from
Nov 16, 2021

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Nov 12, 2021

Summary

This PR implements workflow actions to tell github-bot to run the coverage on main branch and execute the comparison between the coverage results of this PR and the coverage results on main.

This the an example of comment in case there's an increase of coverage:

Test result main count This PR count Difference
Total 17608 17608 0
Passed 16767 16787 ✅ ⏫ +20
Failed 840 820 ✅ ⏬ -20
Panics 1 16787 0
Coverage 95.22% 95.52% +0.30%

And this is an example in case the coverage decreases:

Test result main count This PR count Difference
Total 17608 17608 0
Passed 16767 16687 ❌ ⏬ -80
Failed 840 900 ❌ ⏫ +60
Panics 1 1 0
Coverage 95.22% 95.52% +0.30%

At the moment the command is failing because I changed the logic of the command when using --json as suggested in a previous PR.

Test Plan

I run the commands manually against this PR and the main branch and made sure it works. Also, pasted the results emitted locally.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Nov 12, 2021

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: fb4c491
Status: ✅  Deploy successful!
Preview URL: https://20a44747.tools-8rn.pages.dev

View logs

@github-actions
Copy link

Test262 coverage results

@github-actions
Copy link

Test262 coverage results

Test result main count This PR count Difference
Total 17608 17608 0
Passed 16767 16767 0
Failed 840 840 0
Panics 16767 16767 0
Coverage 95.22% 95.22% 0.00%

@MichaReiser
Copy link
Contributor

Should we add some summary text to the coverage results (with some emoji?) so that it's immediately clear if everything is good or if it requires attention?

@ematipico
Copy link
Contributor Author

Should we add some summary text to the coverage results (with some emoji?) so that it's immediately clear if everything is good or if it requires attention?

Feel free to suggest what you want. At the moment if some number changes, they are highlighted in bold.

@ematipico ematipico marked this pull request as ready for review November 16, 2021 14:00
Copy link
Contributor

@MichaReiser MichaReiser 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. Just make sure to strip out the error changes.

@ematipico ematipico merged commit e7f9a9a into main Nov 16, 2021
@ematipico ematipico deleted the ci/comment-compare branch November 16, 2021 18:49
@github-actions
Copy link

Test262 comparison coverage results on ubuntu-latest

Test result main count This PR count Difference
Total 17608 17608 0
Passed 16767 16767 0
Failed 840 840 0
Panics 1 1 0
Coverage 95.22% 95.22% 0.00%

@github-actions
Copy link

Test262 comparison coverage results on windows-latest

Test result main count This PR count Difference
Total 17608 17608 0
Passed 16767 16767 0
Failed 840 840 0
Panics 1 1 0
Coverage 95.22% 95.22% 0.00%

@github-actions
Copy link

Test262 comparison coverage results on macos-latest

| Test result | main count | This PR count | Difference |'
'| :---------: | :----------: | :-----------: | :--------: |'
'| Total | 17608 | 17608 | 0 |'
'| Passed | 16767 | 16767 | 0 |'
'| Failed | 840 | 840 | 0 |'
'| Panics | 1 | 1 | 0 |'
'| Coverage | 95.22'%' | 95.22'%' | 0.00'%' |

@@ -67,10 +64,55 @@ jobs:
uses: Swatinem/rust-cache@v1
- name: Run Test262 suite on main branch
continue-on-error: true
run: cargo xtask coverage --json
run: cargo xtask coverage --json > base_results.json
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that it now takes quite a while until the results show up on the PR because the coverage command must run twice. Do you think there's a way to publish the result for "main" and store it (e.g. using the commit number in the name) whenever a commit gets merged so that we may just retrieve it (and only run xtask coverage in the case it's missing for whatever reason)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is something I discussed with the others during the standup.

Boa uses Github pages to store the results.

Cloudflare doesn't have a storage service yet. Github actions artifacts are not readable across workflows. Maybe, for now, we should use Github pages

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

Successfully merging this pull request may close these issues.

3 participants