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.

Sign up for GitHub

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

Jump to bottom

generate master branch coverage #2929

Merged
merged 4 commits into from
Sep 27, 2021
Merged

generate master branch coverage #2929

merged 4 commits into from
Sep 27, 2021

Conversation

HarrisChu
Copy link
Contributor

copy from pull_request workflow, just modify trigger.

@HarrisChu HarrisChu added the ready-for-testing PR: ready for the CI test label Sep 26, 2021
yixinglu
yixinglu reviewed Sep 26, 2021
View reviewed changes
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

Could place this job into the nightly workflow

.github/workflows/coverage.yml Outdated Show resolved Hide resolved
.github/workflows/coverage.yml Outdated Show resolved Hide resolved
.github/workflows/coverage.yml Outdated Show resolved Hide resolved
.github/workflows/coverage.yml Outdated
Comment on lines 131 to 134
ubuntu2004)
# ssl cluster
make ENABLE_SSL=true CA_SIGNED=true up
;;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe setup two clusters to test this TLS feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cause it would show diff coverage, it's better to keep consistent with coverage in PR workflow.

Copy link
Contributor

Choose a reason for hiding this comment

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

make sense.

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2021 •
edited

Codecov Report

Merging #2929 (1cd0515) into master (fd2e949) will increase coverage by 0.09%.
The diff coverage is 96.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2929      +/-   ##
==========================================
+ Coverage   85.45%   85.54%   +0.09%     
==========================================
  Files        1225     1229       +4     
  Lines      109855   110378     +523     
==========================================
+ Hits        93872    94426     +554     
+ Misses      15983    15952      -31
Impacted Files Coverage Δ
src/kvstore/EventListener.h 32.22% <0.00%> (-0.19%) ⬇️
src/kvstore/KVEngine.h 100.00% <ø> (ø)
src/kvstore/KVStore.h 66.66% <ø> (ø)
src/kvstore/RocksEngine.h 91.66% <ø> (ø)
src/meta/processors/job/JobDescription.h 100.00% <ø> (ø)
src/meta/processors/job/JobManager.h 100.00% <ø> (ø)
src/mock/AdHocSchemaManager.cpp 56.95% <0.00%> (-0.38%) ⬇️
src/meta/processors/job/AdminJobProcessor.cpp 65.11% <62.50%> (ø)
src/common/function/test/FunctionManagerTest.cpp 98.78% <63.63%> (-0.20%) ⬇️
src/storage/http/StorageHttpPropertyHandler.cpp 78.26% <78.26%> (ø)
... and 74 more

Continue to review full report at Codecov.

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

yixinglu
yixinglu approved these changes Sep 26, 2021
View reviewed changes
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

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

LGTM

@Shylock-Hg
Copy link
Contributor

Why need this?

@HarrisChu
Copy link
Contributor Author

Why need this?

our pr workflow does not run coverage on master branch, just run coverage on commit. (i.e. just on pr, not on push).
if there's no base coverage, codecov could not trigger the pr comments.

@Shylock-Hg
Copy link
Contributor

Shylock-Hg commented Sep 27, 2021 •
edited

Why need this?

our pr workflow does not run coverage on master branch, just run coverage on commit. (i.e. just on pr, not on push).
if there's no base coverage, codecov could not trigger the pr comments.

So, you could add the on_push event on master branch. Nightly may out of date.

Shylock-Hg
Shylock-Hg approved these changes Sep 27, 2021
View reviewed changes
Copy link
Contributor

@Shylock-Hg Shylock-Hg left a comment

Choose a reason for hiding this comment

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

LGTM

@HarrisChu
Copy link
Contributor Author

Why need this?

our pr workflow does not run coverage on master branch, just run coverage on commit. (i.e. just on pr, not on push).
if there's no base coverage, codecov could not trigger the pr comments.

So, you could add the on_push event on master branch. Nightly may out of date.

yes, but right now we have less runner, and it may block other prs.
I will follow this, if we have enough runner, change it to on_push event

@bright-starry-sky bright-starry-sky merged commit b13d4d1 into vesoft-inc:master Sep 27, 2021
@HarrisChu HarrisChu deleted the add_tck_coverage branch September 29, 2021 09:52
@jamieliu1023 jamieliu1023 mentioned this pull request Oct 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yixinglu yixinglu yixinglu approved these changes

@Shylock-Hg Shylock-Hg Shylock-Hg approved these changes

Assignees
No one assigned
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@HarrisChu @codecov-commenter @Shylock-Hg @yixinglu @bright-starry-sky