Skip to content

lcov report #270

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

Merged
merged 6 commits into from
Mar 3, 2025
Merged

lcov report #270

merged 6 commits into from
Mar 3, 2025

Conversation

sdarwin
Copy link
Collaborator

@sdarwin sdarwin commented Feb 27, 2025

No description provided.

Copy link
Collaborator

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

We're disabling GitHub Actions completely?

mkdir -p /tmp/lcov-repo-results || true
skiplist="'

textpart2="${SKIPLIST}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

To make this easier you could use either text+= or just change quotes like

text='use=41"'\
"$variable"'
$non-evaled'

Or use a here-doc and sed with placeholders

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, instead of textpart1 textpart2 textpart3 , and then joining them together, perhaps it all be one monolithic here-doc. Originally, I tried to design it all as one large variable. Then having difficulty embedded variables inside variables, and dealing with multiple quotes inside quotes. So, broke it apart into pieces. That simplified it.

If you strongly prefer to push them all back into one variable again, and be certain it works, with all the quotes and interpolations, then you can modify this PR, or submit another PR, and I will agree. Or, it can stay as-is, I believe it's ok.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking about how to improve this string creation to be "clean".
In the end I think it would be cleanest to NOT have this string at all:

  • add ci/runcodecov.sh as a regular script file
  • before git submodule foreach export the 2 required variables from this script (SKIPLIST & LCOV_SKIP_PATTERN)
  • alternatively pass those as arguments for better visibility and fetch them at the start of the script

Having the script separate allows for easier development, linting (I see you also use shellcheck) and manual execution for focused testing

BTW: I'd suggest to always use set -eu at the top of the scripts to detect misspelled variables, missing parameters and unexpectedly failing commands

The cause was actualy the git safe-directory not the working directory,
so this is not required.
@sdarwin
Copy link
Collaborator Author

sdarwin commented Feb 27, 2025

We're disabling GitHub Actions completely?

@jeking3 , these reports are on a separate branch "feature/reports", to not confuse drone files of the master branch, which are example files for end users.
There is no point in triggering GHA, since that is done on the main branch.

@sdarwin sdarwin force-pushed the report branch 4 times, most recently from 1209c4c to 94f4911 Compare February 27, 2025 19:01
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.23%. Comparing base (63644bc) to head (142c63d).
Report is 6 commits behind head on feature/reports.

❗ There is a different number of reports uploaded between BASE (63644bc) and HEAD (142c63d). Click for more details.

HEAD has 11 uploads less than BASE
Flag BASE (63644bc) HEAD (142c63d)
12 1
Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                  @@
##           feature/reports     #270       +/-   ##
====================================================
- Coverage           100.00%   88.23%   -11.77%     
====================================================
  Files                    2        2               
  Lines                   22       17        -5     
  Branches                11        0       -11     
====================================================
- Hits                    22       15        -7     
- Misses                   0        2        +2     

see 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63644bc...142c63d. Read the comment docs.

mkdir -p /tmp/lcov-repo-results || true
skiplist="'

textpart2="${SKIPLIST}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking about how to improve this string creation to be "clean".
In the end I think it would be cleanest to NOT have this string at all:

  • add ci/runcodecov.sh as a regular script file
  • before git submodule foreach export the 2 required variables from this script (SKIPLIST & LCOV_SKIP_PATTERN)
  • alternatively pass those as arguments for better visibility and fetch them at the start of the script

Having the script separate allows for easier development, linting (I see you also use shellcheck) and manual execution for focused testing

BTW: I'd suggest to always use set -eu at the top of the scripts to detect misspelled variables, missing parameters and unexpectedly failing commands

Required because the owner inside containers is different, likely root,
and git doesn't allow that by default.
@sdarwin sdarwin merged commit d9c55f9 into boostorg:feature/reports Mar 3, 2025
8 of 9 checks passed
@sdarwin sdarwin mentioned this pull request Mar 3, 2025
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.

3 participants