Skip to content

Conversation

@shimkiv
Copy link
Member

@shimkiv shimkiv commented Aug 28, 2024

  • This PR attempts to generalize the complex scripts usage by wrapping them with the Makefile.
    • This will make it impossible to run make command on Windows natively (not sure though if we have users who build stuff on Windows without the WSL).
    • Should we create the Windows CMD/Powershell scripts to emulate some Make targets?
  • CI is updated accordingly to use the Makefile.
    • We need to decide how we want to run tests with code coverage data gathering and reporting. Please refer to internal discussion.
  • Formatting applied against the root .md files.
    • More information and usage examples will be added as part of the followup tasks.
  • Codecov and current repository are configured for coverage reports uploading.

Tasks left:

  • Run it for one specific Rust version only (no reason to do it for whole the matrix).
    • Conditionally run make nextest-with-coverage for one of the rust_toolchain_version: ["1.71", "1.72", "1.73", "1.74"].
    • Extract the make nextest-heavy-with-coverage into Nightly builds with on-demand execution option.
  • Document changes and usage options in CONTRIBUTING.md.
    • CARGO_EXTRA_ARGS="-p poly-commitment" make test-all-with-coverage
    • BIN_EXTRA_ARGS="-p poly-commitment" make nextest-all-with-coverage

@dannywillems
Copy link
Member

Should we create the Windows CMD/Powershell scripts to emulate some Make targets?

No, we should not. It is fine for now, IMO.

Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

LGTM so far. Feel free to continue with the tasks left.
I tested a few make targets.

@codecov
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.18%. Comparing base (efbfd56) to head (68de23a).
Report is 80 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2504      +/-   ##
==========================================
+ Coverage   73.17%   73.18%   +0.01%     
==========================================
  Files         233      233              
  Lines       53846    53885      +39     
==========================================
+ Hits        39401    39437      +36     
- Misses      14445    14448       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… with coverage as part of regular checks, remove flag from coverage uploading to see if it helps with diff processing.
@shimkiv shimkiv mentioned this pull request Aug 29, 2024
Copy link
Member

@dannywillems dannywillems left a comment

Choose a reason for hiding this comment

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

Already approving as it seems it is only related to the linter now.

@shimkiv shimkiv changed the title Add the test coverage data gathering and reports generation. CI refactoring, test coverage data gathering and reports generation. Aug 29, 2024
Released earlier this year in July. No big changes.
Only bumping up to stay up-to-date.
@shimkiv shimkiv requested a review from dannywillems August 30, 2024 13:13
@shimkiv shimkiv marked this pull request as ready for review August 30, 2024 13:13
… and make Nightly builds on demand with the coverage report attached to the job execution results rather than upload it to the Codecov to not mess the diff checks.
…heavy, everything else will be executed during the Nightly runs.
@shimkiv
Copy link
Member Author

shimkiv commented Aug 31, 2024

Hey folks,
let's iterate over this changes later to see if we can improve PR checks timing.

@dannywillems
Copy link
Member

Hey folks, let's iterate over this changes later to see if we can improve PR checks timing.

Agree.

@mrmr1993 mrmr1993 merged commit 3f3897b into master Aug 31, 2024
@mrmr1993 mrmr1993 deleted the chore/test-coverage branch August 31, 2024 18:35
@volhovm
Copy link
Contributor

volhovm commented Sep 17, 2024

@shimkiv is it possible to get this into compatible too?

@shimkiv
Copy link
Member Author

shimkiv commented Sep 17, 2024

@volhovm everything is possible, I'm just thinking which changes you'd like to have in compatible? If we are talking about "quality-of-live" changes only - then no problem. But if you were also thinking to have code coverage uploading to the Codecov service - here we might start to have issues.
Codecov is configured to consider master as default/main branch it will compare coverage against for diff reports. And if compatible is different to main (master) by design, then you might end up with constant "failed" jobs on CI side (unless such comparison will be intentionally disabled).

@volhovm
Copy link
Contributor

volhovm commented Sep 18, 2024

^ (discussed yesterday in PM, but copying): I'd ideally like to see the CI files/Makefile structure synchronized between this repo's master and develop, because rn develop is outdated, a bit broken, and I'm not sure if it makes sense to invest in repairing it if you just wrote a new-and-beautiful CI thing which can override the old-and-boring CI thing :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants