Skip to content

Enable codecov #6067

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

Closed
wants to merge 2 commits into from
Closed

Enable codecov #6067

wants to merge 2 commits into from

Conversation

2010YOUY01
Copy link
Contributor

@2010YOUY01 2010YOUY01 commented Apr 20, 2023

Which issue does this PR close?

Issue #3678
Prev PR that disabled codecov: #3679

Rationale for this change

cargo tarpaulin will consume lots of disk space, previously it failed due to this reason. I tried removing several large files on GitHub Action virtual machines, then It can run again.
Problem: after that, two dataframe tests failed, they're disabled temporarily. (cargo tarpaulin will run tests to collect coverage stats, these tests will only fail under cargo tarpaulin, normal test runs are not affected)
https://github.com/apache/arrow-datafusion/blob/10b0eff7dfbe195a724285bc166b20240e8ebccb/datafusion/core/src/dataframe.rs#L1978
https://github.com/apache/arrow-datafusion/blob/10b0eff7dfbe195a724285bc166b20240e8ebccb/datafusion/core/src/dataframe.rs#L2070
These two test cases are added after last time codecov CI is disabled, I don't know why they might fail under cargo tarpaulin
Failure messages:

  failures:

---- dataframe::tests::partition_aware_union stdout ----
thread 'dataframe::tests::partition_aware_union' panicked at 'assertion failed: `(left == right)`
  left: `2`,
 right: `1`', datafusion/core/src/dataframe.rs:2008:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- dataframe::tests::verify_join_output_partitioning stdout ----
thread 'dataframe::tests::verify_join_output_partitioning' panicked at 'assertion failed: `(left == right)`
  left: `UnknownPartitioning(1)`,
 right: `Hash([Column { name: "c1", index: 0 }, Column { name: "c2", index: 1 }], 1)`', datafusion/core/src/dataframe.rs:2112:21


failures:
    dataframe::tests::partition_aware_union
    dataframe::tests::verify_join_output_partitioning

test result: FAILED. 1475 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 139.30s

Apr 19 21:59:34.770  INFO cargo_tarpaulin::process_handling::linux: Launching test
Apr 19 21:59:34.776  INFO cargo_tarpaulin::process_handling: running /home/runner/work/arrow-datafusion/arrow-datafusion/target/debug/deps/simplification-ba4cc1d732bcee01

Failing CI run : https://github.com/2010YOUY01/arrow-datafusion/actions/runs/4750006102
Can be reproduced with CI config in this commit 75c7fb7

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

- uses: actions/checkout@v3
with:
submodules: true
# `cargo tarpaulin` requires lots of disk space
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change1: rm large unnecessary files

rustup toolchain install stable
rustup default stable
cargo install --version 0.20.1 cargo-tarpaulin
# Disable failing tests temporarily
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change2: skipped failing test cases temporarily

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@10b0eff). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head cdd5f24 differs from pull request most recent head ae31a7b. Consider uploading reports for the commit ae31a7b to get more accurate results

@@           Coverage Diff           @@
##             main    #6067   +/-   ##
=======================================
  Coverage        ?   77.56%           
=======================================
  Files           ?      419           
  Lines           ?    96984           
  Branches        ?        0           
=======================================
  Hits            ?    75228           
  Misses          ?    21756           
  Partials        ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jackwener
Copy link
Member

Just in my opinion, I don't feel the need to add codedev for Datafusion for now.

@2010YOUY01
Copy link
Contributor Author

Just in my opinion, I don't feel the need to add codedev for Datafusion for now.

I followed tarpaulin repo's issues to get this fixed. This tool seems not very stable, people encounter different problems, and even the author can't tell why exactly.
So another issue is if it is got fixed now and it's still likely to break in the future and cause CI problems again :(
Maybe we should leave it commented and If people want to see coverage they can run it themself.

@alamb
Copy link
Contributor

alamb commented Apr 24, 2023

The coverage report from this PR is here https://app.codecov.io/gh/apache/arrow-datafusion/pull/6067 but contains no information on what was not covered (so like what do we get from this site other than a webpage with a number).

When I look at the coverage report from arrow2, it seems to be quite useful (and have sources, etc):
https://app.codecov.io/gh/jorgecarleitao/arrow2?search=&displayType=tree

Maybe we can follow the model there https://github.com/jorgecarleitao/arrow2/blob/main/.github/workflows/coverage.yml 🤔

@2010YOUY01
Copy link
Contributor Author

Detailed information is normally displayed when switching to commit instead of pull requests.
https://app.codecov.io/gh/apache/arrow-datafusion/tree/enable-codecov
I guess for PRs' reports, Codecov requires the latest commit of the main branch to already have a code coverage report in order to use it as a base for comparison. That might be the reason for not showing a detailed report in the first PR.🤔
And thank you for the pointer! I will check if the tool they used to generate Codecov reports is more stable than the current one.

@alamb
Copy link
Contributor

alamb commented Apr 24, 2023

That might be the reason for not showing a detailed report in the first PR.🤔

It is possible.

However, if you look at codecov reports from sqlparser (which I think is configured the same as this), such as the one on apache/datafusion-sqlparser-rs#817 https://coveralls.io/builds/57556726

it has the same lack of source issue

@2010YOUY01 2010YOUY01 marked this pull request as draft April 24, 2023 17:36
Copy link

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale PR has not had any activity for some time label May 11, 2024
@github-actions github-actions bot closed this May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale PR has not had any activity for some time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants