Uh oh!
There was an error while loading. Please reload this page.
ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage - #9262
ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage#9262alamb wants to merge 2 commits into
Conversation
1e5dc72 to
f0b002fCompareThere was a problem hiding this comment.
this appears to run all the arrow tests twice -- once in the main workspace and once in the arrow crate again. I am not sure that is adding any value
There was a problem hiding this comment.
This was introduced when I was trying to test the SIMD and non-SIMD versions of the code. Feature options were not being respected within the sub-crates when supplied at the workspace level, see here. So the solution was to move into the sub-crate to enable/disable the feature.
Quickly reading through the cargo issues, it's not clear that this has been resolved yet.
There was a problem hiding this comment.
AH, that makes sense @paddyhoran -- I think the feature flag test for simd got moved to an entire different job, namely the linux-test-simd job below. I think I need to re-instante the second CI test but run the tests with the pretty print option. I'll try and do that tomorrow
There was a problem hiding this comment.
In fact the CI test failed, exactly as you predicted https://github.com/apache/arrow/runs/1727917047 -- namely that you can't specify --features in the root of the workspace:
Run export CARGO_HOME="/github/home/.cargo"
error: --features is not allowed in the root of a virtual workspace
note: while this was previously accepted, it didn't actually do anything
help: change the current directory to the package directory, or use the --manifest-path flag to the path of the package
Error: Process completed with exit code 101.
There was a problem hiding this comment.
A solution could be to exclude arrow from the initial cargo test, then test it after cd arrow, with the prettyprint (and other future) feature.
Or has this been resolved?
There was a problem hiding this comment.
It has been resolved (though the resolution was to keep running the tests for arrow twice)
There was a problem hiding this comment.
To be fair, running the arrow tests is a second or so, so the harm is pretty small. Re-compiling DataFusion's dependencies would have been a different beast ^_^
There was a problem hiding this comment.
Is everyone ok if I merge this in I think it will have minimal effect on overall test times (will require arrow to get recompiled one additional time I think)
Codecov Report
@@ Coverage Diff @@## master #9262 +/- ##
=======================================
Coverage 81.62% 81.62% =======================================
Files 215 215 Lines 52520 52520 =======================================
+ Hits 42870 42871 +1 + Misses 9650 9649 -1
Continue to review full report at Codecov.
|
f0b002f to
063a7c8CompareYou can now see that the pretty print tests are running:
|
This PR changes the Rust CI job to include the prettyprint feature. (so we have CI coverage of that feature) As @paddyhoran noted, passing in feature flags to the root workspace doesn't do anything, and as it turns out we already invoke `cargo test` for the `arrow` crate separately. Prior to this PR I think that just ran the same tests again with the same features. After this PR, the second run of the arrow tests are run with the `prettyprint` feature Here is evidence that the tests are running twice. I picked a PR (randomly) https://github.com/apache/arrow/pull/9258/checks?check_run_id=1725967358 If you look at the logs for the run-tests action  You can see that the same test is run twice: ``` 2021-01-19T07:02:49.9476111Z test compute::kernels::boolean::tests::test_nonnull_array_is_not_null ... ok 2021-01-19T07:02:49.9476854Z test compute::kernels::boolean::tests::test_nonnull_array_is_null ... ok 2021-01-19T07:02:49.9477616Z test compute::kernels::boolean::tests::test_nonnull_array_with_offset_is_not_null ... ok 2021-01-19T07:02:49.9478427Z test compute::kernels::boolean::tests::test_nonnull_array_with_offset_is_null ... ok 2021-01-19T07:02:49.9479207Z test compute::kernels::boolean::tests::test_nullable_array_is_null ... ok 2021-01-19T07:02:49.9479948Z test compute::kernels::boolean::tests::test_nullable_array_is_not_null ... ok 2021-01-19T07:02:49.9480744Z test compute::kernels::boolean::tests::test_nullable_array_with_offset_is_not_null ... ok 2021-01-19T07:02:49.9481640Z test compute::kernels::boolean::tests::test_nullable_array_with_offset_is_null ... ok 2021-01-19T07:02:49.9487019Z test compute::kernels::boolean::tests::test_nullif_int_array_offset ... ok 2021-01-19T07:02:49.9487773Z test compute::kernels::boolean::tests::test_nullif_int_array ... ok ... 2021-01-19T07:07:23.4568865Z test compute::kernels::boolean::tests::test_nonnull_array_is_not_null ... ok 2021-01-19T07:07:23.4569576Z test compute::kernels::boolean::tests::test_nonnull_array_is_null ... ok 2021-01-19T07:07:23.4570337Z test compute::kernels::boolean::tests::test_nonnull_array_with_offset_is_not_null ... ok 2021-01-19T07:07:23.4571133Z test compute::kernels::boolean::tests::test_nonnull_array_with_offset_is_null ... ok 2021-01-19T07:07:23.4571885Z test compute::kernels::boolean::tests::test_nullable_array_is_not_null ... ok 2021-01-19T07:07:23.4572614Z test compute::kernels::boolean::tests::test_nullable_array_is_null ... ok 2021-01-19T07:07:23.4573383Z test compute::kernels::boolean::tests::test_nullable_array_with_offset_is_not_null ... ok 2021-01-19T07:07:23.4574183Z test compute::kernels::boolean::tests::test_nullable_array_with_offset_is_null ... ok 2021-01-19T07:07:23.4574929Z test compute::kernels::boolean::tests::test_nullif_int_array ... ok 2021-01-19T07:07:23.4575636Z test compute::kernels::boolean::tests::test_nullif_int_array_offset ... ok ``` Closes#9262 from alamb/alamb/run_prettyprint_ci Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>
codecov-commenter
commented
Aug 20, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## master #9262 +/- ##
=======================================
Coverage 81.62% 81.62% =======================================
Files 215 215 Lines 52520 52520 =======================================
+ Hits 42870 42871 +1 + Misses 9650 9649 -1 ☔ View full report in Codecov by Sentry. |
This PR changes the Rust CI job to include the prettyprint feature. (so we have CI coverage of that feature)
As @paddyhoran noted, passing in feature flags to the root workspace doesn't do anything, and as it turns out we already invoke
cargo testfor thearrowcrate separately. Prior to this PR I think that just ran the same tests again with the same features. After this PR, the second run of the arrow tests are run with theprettyprintfeatureHere is evidence that the tests are running twice. I picked a PR (randomly)
https://github.com/apache/arrow/pull/9258/checks?check_run_id=1725967358
If you look at the logs for the run-tests action
You can see that the same test is run twice: