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 scan #5806

Merged
merged 1 commit into from
Jul 20, 2023
Merged

common: add coverage scan #5806

merged 1 commit into from
Jul 20, 2023

Conversation

janekmi
Copy link
Contributor

@janekmi janekmi commented Jul 18, 2023

This change is Reviewable

@janekmi janekmi added the sprint goal This pull request is part of the ongoing sprint label Jul 18, 2023
@janekmi janekmi added this to the 2.0.0 milestone Jul 18, 2023
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #5806 (f16d884) into master (1527cfc) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5806      +/-   ##
==========================================
+ Coverage   70.97%   70.99%   +0.01%     
==========================================
  Files         131      131              
  Lines       19175    19175              
  Branches     3193     3192       -1     
==========================================
+ Hits        13610    13613       +3     
+ Misses       5565     5562       -3     

Copy link
Contributor

@grom72 grom72 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, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @janekmi)

a discussion (no related file):
Shall we remove COVERAGE=1 from ubuntu.yml?



.github/workflows/scan_coverage.yml line 5 at r1 (raw file):

on:
  workflow_dispatch:

Let's make it simply sub-workflow


.github/workflows/scan_coverage.yml line 19 at r1 (raw file):

  OS_VER:       22.04
  COVERAGE:     1
  TEST_BUILD:   DEBUG

debug??

Code quote:

DEBUG

Copy link
Contributor Author

@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 1 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @grom72)

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Shall we remove COVERAGE=1 from ubuntu.yml?

Actually, we agreed to keep it there since we still would like to see coverage change per pull request.



.github/workflows/scan_coverage.yml line 5 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

Let's make it simply sub-workflow

I recognize that sometimes we might need to run it manually to update the coverage after a more impactful change so pull requests coming later the same day would yield more accurate coverage results.


.github/workflows/scan_coverage.yml line 19 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

debug??

Good point. I think we can discuss it tomorrow. Till now coverage was always produced using debug builds.
But we know that these builds introduce additional logic e.g. more checks which may impact the final result.
I would say in a negative way. However, I would not expect a significantly different result when coverage would be measured using a nondebug build.

@janekmi janekmi added the build system What Makefiles do label Jul 18, 2023
Copy link
Contributor Author

@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 1 of 2 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @grom72)

a discussion (no related file):
Preview: https://github.com/pmem/pmdk/actions/runs/5589044785/jobs/10216738281
Result: https://app.codecov.io/github/pmem/pmdk/commit/b6f63c6b52b1dac961d3debc80cdb30bc7c62119


Copy link
Contributor

@grom72 grom72 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: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

Actually, we agreed to keep it there since we still would like to see coverage change per pull request.

In that case, what is the added value of this scan?



.github/workflows/scan_coverage.yml line 19 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Good point. I think we can discuss it tomorrow. Till now coverage was always produced using debug builds.
But we know that these builds introduce additional logic e.g. more checks which may impact the final result.
I would say in a negative way. However, I would not expect a significantly different result when coverage would be measured using a nondebug build.

I mean debug (lowercase) instead of DEBUG (uppercase).

Copy link
Contributor

@grom72 grom72 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: all files reviewed, 2 unresolved discussions (waiting on @janekmi)


.github/workflows/scan_coverage.yml line 5 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I recognize that sometimes we might need to run it manually to update the coverage after a more impactful change so pull requests coming later the same day would yield more accurate coverage results.

But in such case, we have a long All workflows list on Actions (https://github.com/pmem/pmdk/actions) page.

Copy link
Contributor

@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.

:lgtm:

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

Copy link
Contributor Author

@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.

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @grom72 and @osalyk)

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

In that case, what is the added value of this scan?

Oh, it turned out to be a really good question. I thought that since the main.yml is not triggered by push event the codecov.io report on the master branch is not being updated. But I am not 100% sure this is the case.

Let's discuss it offline.



.github/workflows/scan_coverage.yml line 5 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

But in such case, we have a long All workflows list on Actions (https://github.com/pmem/pmdk/actions) page.

Can we discuss functionality before aesthetics?


.github/workflows/scan_coverage.yml line 19 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

I mean debug (lowercase) instead of DEBUG (uppercase).

Done.

Copy link
Contributor

@grom72 grom72 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 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi)

Copy link
Contributor

@grom72 grom72 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: all files reviewed, 2 unresolved discussions (waiting on @janekmi)


.github/workflows/scan_coverage.yml line 15 at r2 (raw file):

  PMDK_CC:      gcc
  PMDK_CXX:     g++
  VALGRIND:     0

We have to remember to use the same VALGRIND value for a similar workflow on pull_request.
Can we reuse this file there? I mean as an extension to #5807

Code quote:

VALGRIND:     0

Copy link
Contributor Author

@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 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)


.github/workflows/scan_coverage.yml line 15 at r2 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

We have to remember to use the same VALGRIND value for a similar workflow on pull_request.
Can we reuse this file there? I mean as an extension to #5807

We can consider using fromJSON() function providing the JSON e.g. from the repository variable.

Ref: https://docs.github.com/en/actions/learn-github-actions/expressions#fromjson
Ref: https://github.com/pmem/pmdk/settings/variables/actions/new

FYI @osalyk

Copy link
Contributor

@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.

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)

Copy link
Contributor

@grom72 grom72 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: all files reviewed, 1 unresolved discussion (waiting on @janekmi)


.github/workflows/scan_coverage.yml line 15 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

We can consider using fromJSON() function providing the JSON e.g. from the repository variable.

Ref: https://docs.github.com/en/actions/learn-github-actions/expressions#fromjson
Ref: https://github.com/pmem/pmdk/settings/variables/actions/new

FYI @osalyk

I mean add something like this to main.yml:

  call-coverage:
    uses: ./.github/workflows/scan_coverage.yml
    name: Coverage

Copy link
Contributor

@grom72 grom72 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

Copy link
Contributor Author

@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 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

Copy link
Contributor Author

@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 r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

Copy link
Contributor

@grom72 grom72 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 r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

@janekmi janekmi merged commit 1d3aa68 into pmem:master Jul 20, 2023
6 checks passed
Signed-off-by: Jan Michalski <jan.michalski@intel.com>
@janekmi janekmi removed the build system What Makefiles do label Aug 31, 2023
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.

3 participants