pcaversaccio / createx

Factory smart contract to make easier and safer usage of the `CREATE` and `CREATE2` EVM opcodes as well as of `CREATE3`-based (i.e. without an initcode factor) contract creations.
https://createx.rocks
GNU Affero General Public License v3.0
342 stars 26 forks source link

👷‍♂️ Refactor Coverage Report PR Commenting #37

Closed pcaversaccio closed 11 months ago

pcaversaccio commented 12 months ago

🕓 Changelog

This PR introduces a check if the PR is based on an internal non-main branch and will only post in this case a coverage report comment. Since lcov-reporter-action doesn't support the pull_request_target event type (see here), it won't create a comment if I would configure it accordingly (for fork-based PRs we would need to use pull_request_target as event type to enable fork-based PRs running the coverage report commenting successfully since they need access to the GITHUB_TOKEN in our case). That's why I discard doing though. But the full coverage report is printed in the action summary, and thus we can always verify the coverage for fork-based PRs. Furthermore, all the minimum coverage (in our case 100%) is now also verified in case of a push (if we directly push to main for some reason) or workflow_dispatch trigger.

Note, this PR also bumps the zgosalvez/github-actions-report-lcov action to v4 (https://github.com/pcaversaccio/createx/pull/37/commits/d7d76225f33a8f089851e7a198f3ec870bbfd3df).

This PR is rebased on #36.

🐶 Cute Animal Picture

image

mds1 commented 11 months ago

Coverage report changes LGTM, but not approving to prevent an accidental merge since this also contains the nonce change from #36 that we might change per https://github.com/pcaversaccio/createx/pull/36#discussion_r1410930753

pcaversaccio commented 11 months ago

Coverage report changes LGTM, but not approving to prevent an accidental merge since this also contains the nonce change from #36 that we might change per #36 (comment)

PR is again rebased on the latest changes that include the fix with vm.setNonceUnsafe as well as I bumped an outdated action (https://github.com/pcaversaccio/createx/pull/37/commits/d7d76225f33a8f089851e7a198f3ec870bbfd3df).

github-actions[bot] commented 11 months ago

Coverage after merging fix-fork-ci into main will be

100.00%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   CreateX.sol100%100%100%100%
pcaversaccio commented 11 months ago

Note: The code changes committed to this PR have been merged as part of https://github.com/pcaversaccio/createx/pull/40.