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

coverage report untested files (and misc cleanup) #10556

Merged
merged 6 commits into from
Nov 23, 2024
Merged

Conversation

turadg
Copy link
Member

@turadg turadg commented Nov 22, 2024

incidental

Description

The main change in this PR is to make the code coverage reports include files that aren't tested at all (or apparently aren't due to the bundling problem #1817)

It also cleans up a bunch of obsolete stuff about NESM and adds some more usage docs.

Security Considerations

none

Scaling Considerations

n/a

Documentation Considerations

Improves

Testing Considerations

I ran the commands locally to verify the reports. Once this lands they'll appear at https://agoric-sdk-coverage.netlify.app/

Upgrade Considerations

n/a

@turadg turadg requested a review from a team as a code owner November 22, 2024 22:56
@turadg turadg added automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR and removed bypass:integration Prevent integration tests from running on PR labels Nov 22, 2024
Copy link

cloudflare-workers-and-pages bot commented Nov 22, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8efce30
Status: ✅  Deploy successful!
Preview URL: https://b676ca46.agoric-sdk.pages.dev
Branch Preview URL: https://ta-codecov-cleanup.agoric-sdk.pages.dev

View logs

Copy link
Member

@0xpatrickdev 0xpatrickdev left a comment

Choose a reason for hiding this comment

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

The c8 changes look good. I did not audit the yarn-dedupe changes closely but trust you know what you're doing.

@turadg turadg force-pushed the ta/codecov-cleanup branch from 006ad76 to f22763f Compare November 22, 2024 23:41
@turadg turadg force-pushed the ta/codecov-cleanup branch from f22763f to 8efce30 Compare November 22, 2024 23:59
@mergify mergify bot merged commit 5cfb99b into master Nov 23, 2024
81 checks passed
@mergify mergify bot deleted the ta/codecov-cleanup branch November 23, 2024 00:43
@turadg
Copy link
Member Author

turadg commented Nov 23, 2024

Turns out the master CI job that calls test:c8-all depended on C8_OPTIONS. Fix in the works.

@turadg turadg mentioned this pull request Nov 23, 2024
mergify bot added a commit that referenced this pull request Nov 23, 2024
followup: #10556

## Description
#10556 removed C8_OPTIONS without looking closely at the fence. That parameter was used by the `test:c8-all` command to stitch together the package coverage collections.

This restores that and adds some documentation.

### Security Considerations
none
### Scaling Considerations
none
### Documentation Considerations
none
### Testing Considerations
[manual trigger the after-merge branch](https://github.com/Agoric/agoric-sdk/actions/runs/11984250775) to verify the build output at https://agoric-sdk-coverage.netlify.app/ before merging. UPDATE: `coverage` only runs on `push` so the manual trigger didn't work. I suppose so a random branch doesn't overwrite the master coverage report.

### Upgrade Considerations
none
mergify bot added a commit that referenced this pull request Nov 23, 2024
refs: #10559

## Description

#10556 made coverage reports include all files, but only in package reports. This makes CI's report across all packages also use `--all`.

While working on this I wanted to be able to document all the kinks I ran into so I also consolidated this capability in a single script with explanatory comments. 

### Security Considerations
none

### Scaling Considerations
none

### Documentation Considerations
includes docs now

### Testing Considerations
The `coverage` job  runs in the after-merge workflow but only on `push` so I can't trigger it in CI. Instead I've run the script locally. When it reaches cosmic-swingset it dies on `./bin/rosetta-cli` but I just skipped that package and confirmed the generated report is as expected.

### Upgrade Considerations
n/a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants