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

fix: test packages and coverage generation #966

Merged
merged 18 commits into from
Nov 15, 2022

Conversation

rahulghangas
Copy link
Contributor

@rahulghangas rahulghangas commented Nov 7, 2022

@codecov-commenter
Copy link

codecov-commenter commented Nov 7, 2022

Codecov Report

Merging #966 (d68780d) into main (8204e41) will increase coverage by 24.00%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main     #966       +/-   ##
===========================================
+ Coverage   27.08%   51.09%   +24.00%     
===========================================
  Files          81       71       -10     
  Lines        9067     4378     -4689     
===========================================
- Hits         2456     2237      -219     
+ Misses       6375     1915     -4460     
+ Partials      236      226       -10     
Impacted Files Coverage Δ
x/blob/types/tx.pb.go
x/qgb/types/query.pb.go
x/qgb/types/genesis.pb.go
x/blob/types/tx.pb.gw.go
x/qgb/types/query.pb.gw.go
x/qgb/types/types.pb.go
x/blob/types/params.pb.go
x/blob/types/query.pb.go
x/blob/types/query.pb.gw.go
x/blob/types/genesis.pb.go

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rahulghangas rahulghangas marked this pull request as ready for review November 7, 2022 06:50
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

[blocking][question] sorry for the naive question but how do we know that code coverage is fixed with this PR? Is there a PR on your fork that demonstrates what the comment for coverage looks like after this PR is merged?

#966 (comment) doesn't appear exhaustive of all packages in this repo.

.github/workflows/test.yml Outdated Show resolved Hide resolved
Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking care of this 👍 👍
I left a blocking comment, but since I am not very familiar with uploading test coverage, I might be wrong :D

.github/workflows/test.yml Outdated Show resolved Hide resolved
@rach-id rach-id added the CI item that directly relates to or modify the CI label Nov 7, 2022
@rahulghangas
Copy link
Contributor Author

rahulghangas commented Nov 10, 2022

@rootulp https://app.codecov.io/github/celestiaorg/celestia-app/tree/chore%2Ffix-coverage

#966 (comment) was generated by the buggy script, was resolved in a later commit

rach-id
rach-id previously approved these changes Nov 10, 2022
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

[blocking]
Code coverage on main: https://app.codecov.io/github/celestiaorg/celestia-app/tree/main/ has 8199 lines covered
Code coverage on this branch: https://app.codecov.io/github/celestiaorg/celestia-app/tree/chore%2Ffix-coverage/ has 4845 lines covered

Why does this branch have fewer lines covered? Can it be rebased on top of current main to see what the difference is? If this PR fixes the issue, I would expect more lines to be covered, not less.

.github/workflows/test.yml Outdated Show resolved Hide resolved
Comment on lines 153 to 158
excludelist="$(find . -type f -name '*.go' | xargs grep -l 'DONTCOVER')\n"
excludelist+="$(find . -type f -name '*.pb.*')\n"
excludelist+="$(find . -type f -path './tests/mocks/*.go')"
for filename in ${excludelist}; do
filename=$(echo $filename | sed 's/^./github.com\/cosmos\/cosmos-sdk/g')
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's not clear to me what the fix is. Can you please elaborate in the PR description or a code comment?

@rach-id
Copy link
Member

rach-id commented Nov 10, 2022

@rootulp If I understand right:

Code coverage on main: https://app.codecov.io/github/celestiaorg/celestia-app/tree/main/ has 8199 lines covered
Code coverage on this branch: https://app.codecov.io/github/celestiaorg/celestia-app/tree/chore%2Ffix-coverage/ has 4845 lines covered

Probably, because the proto generated files are not included.

@rootulp
Copy link
Collaborator

rootulp commented Nov 10, 2022

Probably, because the proto generated files are not included.

https://github.com/celestiaorg/celestia-app/pull/966/files#diff-faff1af3d8ff408964a57b2e475f69a6b7c7b71c9978cccc8f471798caac2c88R153-R155 removed

          excludelist+=" $(find ./ -type f -name '*.pb.gw.go')"

from the exclude list so I'd expect more proto files to be included in the coverage.txt

@rach-id
Copy link
Member

rach-id commented Nov 10, 2022

@rootulp I guess it's these files that are excluded:

          excludelist="$(find . -type f -name '*.go' | xargs grep -l 'DONTCOVER')\n"
          excludelist+="$(find . -type f -name '*.pb.*')\n"
          excludelist+="$(find . -type f -path './tests/mocks/*.go')"

As per:

            sed -i.bak "/$(echo $filename | sed 's/\//\\\//g')/d" coverage.txt

Or, I am missing something

@rootulp
Copy link
Collaborator

rootulp commented Nov 10, 2022

It looks like before and after this PR, .pb.go files are being covered:

Fewer .pb.go files show up for this branch but I don't think any are intended.

No .pb.gw.go files are being covered in this branch (which seems desirable):

@rahulghangas
Copy link
Contributor Author

rahulghangas commented Nov 10, 2022

I don't get why it's picking up those files, or why changing it would help since the command above returns all the *pb.go files successfully when I run it locally

evan-forbes
evan-forbes previously approved these changes Nov 14, 2022
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

LGTM utACK

thx for this @rahulghangas !

@rahulghangas
Copy link
Contributor Author

rahulghangas commented Nov 14, 2022

How do I go about triggering a code coverage report generation?

@rach-id
Copy link
Member

rach-id commented Nov 14, 2022

If I understand right, codecov will automatically push a comment if there were some changes, for the first time the PR is open. Then, it will start updating the comment, it posted the first time, every time a commit does a change to the patterns:

            **/**.go
            go.mod
            go.sum

However, we can always access the coverage from here: https://app.codecov.io/github/celestiaorg/celestia-app/tree/chore%2Ffix-coverage/

To trigger a new coverage report, changes need to be done to the above files. Probably, a dummy commit and revert would do the job. Or, someone with admin access (don't know which permissions they need to have).

This reverts commit d68780d.
@rahulghangas
Copy link
Contributor Author

rahulghangas commented Nov 15, 2022

https://app.codecov.io/github/celestiaorg/celestia-app/tree/chore%2Ffix-coverage/
All proto files are excluded correctly now

ps - I hate bash!

Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

@rahulghangas Thanks a lot for handling this 👍 👍 🎉 🎉

@rahulghangas rahulghangas merged commit b81723b into celestiaorg:main Nov 15, 2022
@rahulghangas rahulghangas deleted the chore/fix-coverage branch November 15, 2022 10:27
rach-id pushed a commit to rach-id/celestia-app that referenced this pull request Nov 16, 2022
rach-id pushed a commit to rach-id/celestia-app that referenced this pull request Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI item that directly relates to or modify the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix test coverage script
5 participants