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

common: add coverage build #5830

Merged
merged 1 commit into from
Jul 28, 2023
Merged

common: add coverage build #5830

merged 1 commit into from
Jul 28, 2023

Conversation

osalyk
Copy link
Contributor

@osalyk osalyk commented Jul 27, 2023

This change is Reviewable

@osalyk osalyk added the sprint goal This pull request is part of the ongoing sprint label Jul 27, 2023
@osalyk osalyk changed the title common: dd coverage build common: add coverage build Jul 27, 2023
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #5830 (7f578ee) into master (b0a3f84) will increase coverage by 70.99%.
The diff coverage is n/a.

❗ Current head 7f578ee differs from pull request most recent head 189a2c7. Consider uploading reports for the commit 189a2c7 to get more accurate results

@@             Coverage Diff             @@
##           master    #5830       +/-   ##
===========================================
+ Coverage        0   70.99%   +70.99%     
===========================================
  Files           0      131      +131     
  Lines           0    19175    +19175     
  Branches        0     3193     +3193     
===========================================
+ Hits            0    13614    +13614     
- Misses          0     5561     +5561     

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk)


.github/workflows/ubuntu.yml line 28 at r2 (raw file):

    strategy:
      matrix:
        TEST_BUILD: ['debug', 'nondebug'] 

There is redundant space at the end of the line. Please remove.


utils/create-testconfig.sh line 49 at r2 (raw file):

# static values for both Bash and Python
KEEP_GOING=n

I think this change has to be considered in the wider context. e.g. this script is also used for Valgrind and long tests in which cases we might actually prefer to keep KEEP_GOING=y. I think it eventually will become a regular parameter for this script like OUTPUT_DIR with some reasonable default.

But for the sake of this change, it is unnecessary to change this behaviour as well. We have established off-line that COVERAGE=1 builds do not benefit from changing this setting anyway.

@osalyk osalyk force-pushed the add-coverage-build branch 2 times, most recently from e32bc96 to 189a2c7 Compare July 28, 2023 06:47
Copy link
Contributor Author

@osalyk osalyk left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @janekmi)


.github/workflows/ubuntu.yml line 28 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

There is redundant space at the end of the line. Please remove.

Done.


utils/create-testconfig.sh line 49 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think this change has to be considered in the wider context. e.g. this script is also used for Valgrind and long tests in which cases we might actually prefer to keep KEEP_GOING=y. I think it eventually will become a regular parameter for this script like OUTPUT_DIR with some reasonable default.

But for the sake of this change, it is unnecessary to change this behaviour as well. We have established off-line that COVERAGE=1 builds do not benefit from changing this setting anyway.

Done.

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

@janekmi janekmi merged commit efe7cb0 into pmem:master Jul 28, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sprint goal This pull request is part of the ongoing sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants