-
Notifications
You must be signed in to change notification settings - Fork 993
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
Centralize GitHub workflows and add code coverage #17481
base: develop2
Are you sure you want to change the base?
Conversation
a84039c
to
70ddc5b
Compare
2e8f822
to
e32874e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop2 #17481 +/- ##
===========================================
Coverage ? 89.36%
===========================================
Files ? 247
Lines ? 22035
Branches ? 0
===========================================
Hits ? 19692
Misses ? 2343
Partials ? 0 |
35a052b
to
4d1363b
Compare
setup.cfg
Outdated
/Users/runner/**/conan | ||
/home/runner/**/conan | ||
/__w/**/conan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why these weird paths with /runner/?
Why the conans
path is not included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a tricky behavior of coverage
tool. Coverage files are generated across different OS and machines, thus, coverage reports will contain different absolute paths to reference to the same project relative paths.
These absolute "regex" paths are needed by coverage
to merge those reports into a single one.
The first path has to be the relative path to the project, in this case, conan
. Conan is interpreted as conan
main git folder, not the conan
subfolder so it should be able to merge other paths such as conans
and test
.
Anyway, I'm running a quick test in a workflow right now to validate this last statement.
Useful links:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end we didn't need to define the path
option as there exists a relative_files
option which tells coverage to generate file reports relative to project path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the issue with the weird paths. Otherwise, if it only builds for develop2 branch, assigning it to @czoido
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
8d2bff6
to
3202c93
Compare
@@ -27,7 +27,7 @@ runs: | |||
using: 'composite' | |||
steps: | |||
- name: Run tests with coverage | |||
shell: bash | |||
shell: ${{ runner.os == 'Windows' && 'pwsh' || 'bash' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
running tests on powershell on windows can affect the result of the tests, as things that conan generates, as virtualenv files, etc, won't work for powershell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since 2019, powershell is the default shell in windows runners in GitHub.
This change just tries to emulates what we have doing in the past before moving pytest
execution to a GitHub composite action.
A support example:
https://github.com/conan-io/conan/actions/runs/12906022606/job/35986463253#step:18:34
This is a recent windows functional test workflow where it can be seen that pwsh
is the shell in usage by the windows job.
Changelog: Omit
Docs: Omit
Merge workflows, recover coverage from all test results and report them.