You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/01/19 12:49:17 UTC
[GitHub] [arrow] alamb opened a new pull request #9262: ARROW-11317: Run tests once in CI (not twice) and include prettyprint feature
alamb opened a new pull request #9262:
URL: https://github.com/apache/arrow/pull/9262
This PR changes the Rust CI to:
1. Run tests including the `prettyprint` feature (so we have CI coverage of that feature)
2. Run the test suite only once
I don't see any reason to run the test suite twice, though perhaps I am missing something.
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
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] codecov-io edited a comment on pull request #9262: ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#issuecomment-762843890
# [Codecov](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=h1) Report
> Merging [#9262](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=desc) (063a7c8) into [master](https://codecov.io/gh/apache/arrow/commit/e7c69e638e5152e22567a46f19017814dcbe3d7f?el=desc) (e7c69e6) will **increase** coverage by `0.00%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9262 +/- ##
=======================================
Coverage 81.62% 81.62%
=======================================
Files 215 215
Lines 52520 52520
=======================================
+ Hits 42870 42871 +1
+ Misses 9650 9649 -1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rust/parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow/pull/9262/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9lbmNvZGluZ3MvZW5jb2RpbmcucnM=) | `95.05% <0.00%> (+0.19%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=footer). Last update [e7c69e6...063a7c8](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] paddyhoran commented on a change in pull request #9262: ARROW-11317: Run tests once in CI (not twice) and include prettyprint feature
Posted by GitBox <gi...@apache.org>.
paddyhoran commented on a change in pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#discussion_r560352119
##########
File path: .github/workflows/rust.yml
##########
@@ -109,14 +109,14 @@ jobs:
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd rust
- cargo test
+ # run tests on all workspace members
+ cargo test --all --features=prettyprint
# test datafusion examples
cd datafusion
cargo run --example csv_sql
cargo run --example parquet_sql
cd ..
cd arrow
- cargo test
Review 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](https://github.com/rust-lang/cargo/issues/5015#issuecomment-532483775). 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9262: ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#discussion_r562702562
##########
File path: .github/workflows/rust.yml
##########
@@ -109,14 +109,14 @@ jobs:
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd rust
- cargo test
+ # run tests on all workspace members
+ cargo test --all --features=prettyprint
# test datafusion examples
cd datafusion
cargo run --example csv_sql
cargo run --example parquet_sql
cd ..
cd arrow
- cargo test
Review comment:
It has been resolved (though the resolution was to keep running the tests for arrow twice)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9262: ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#discussion_r563109574
##########
File path: .github/workflows/rust.yml
##########
@@ -109,14 +109,14 @@ jobs:
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd rust
- cargo test
+ # run tests on all workspace members
+ cargo test --all --features=prettyprint
# test datafusion examples
cd datafusion
cargo run --example csv_sql
cargo run --example parquet_sql
cd ..
cd arrow
- cargo test
Review 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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9262: ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#discussion_r560898665
##########
File path: .github/workflows/rust.yml
##########
@@ -109,14 +109,14 @@ jobs:
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd rust
- cargo test
+ # run tests on all workspace members
+ cargo test --all --features=prettyprint
# test datafusion examples
cd datafusion
cargo run --example csv_sql
cargo run --example parquet_sql
cd ..
cd arrow
- cargo test
Review 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.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on a change in pull request #9262: ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#discussion_r562857366
##########
File path: .github/workflows/rust.yml
##########
@@ -109,14 +109,14 @@ jobs:
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd rust
- cargo test
+ # run tests on all workspace members
+ cargo test --all --features=prettyprint
# test datafusion examples
cd datafusion
cargo run --example csv_sql
cargo run --example parquet_sql
cd ..
cd arrow
- cargo test
Review 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 ^_^
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9262: ARROW-11317: Run tests once in CI (not twice) and include prettyprint feature
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#discussion_r560155736
##########
File path: .github/workflows/rust.yml
##########
@@ -109,14 +109,14 @@ jobs:
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd rust
- cargo test
+ # run tests on all workspace members
+ cargo test --all --features=prettyprint
# test datafusion examples
cd datafusion
cargo run --example csv_sql
cargo run --example parquet_sql
cd ..
cd arrow
- cargo test
Review 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #9262: ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#discussion_r562623291
##########
File path: .github/workflows/rust.yml
##########
@@ -109,14 +109,14 @@ jobs:
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd rust
- cargo test
+ # run tests on all workspace members
+ cargo test --all --features=prettyprint
# test datafusion examples
cd datafusion
cargo run --example csv_sql
cargo run --example parquet_sql
cd ..
cd arrow
- cargo test
Review 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb edited a comment on pull request #9262: ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage
Posted by GitBox <gi...@apache.org>.
alamb edited a comment on pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#issuecomment-763597193
You can now see that the pretty print tests are running:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9262: ARROW-11317: Run tests once in CI (not twice) and include prettyprint feature
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#issuecomment-762826498
https://issues.apache.org/jira/browse/ARROW-11317
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9262: ARROW-11317: Run tests once in CI (not twice) and include prettyprint feature
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#discussion_r560545792
##########
File path: .github/workflows/rust.yml
##########
@@ -109,14 +109,14 @@ jobs:
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd rust
- cargo test
+ # run tests on all workspace members
+ cargo test --all --features=prettyprint
# test datafusion examples
cd datafusion
cargo run --example csv_sql
cargo run --example parquet_sql
cd ..
cd arrow
- cargo test
Review 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao closed pull request #9262: ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage
Posted by GitBox <gi...@apache.org>.
jorgecarleitao closed pull request #9262:
URL: https://github.com/apache/arrow/pull/9262
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] codecov-io commented on pull request #9262: ARROW-11317: Run tests once in CI (not twice) and include prettyprint feature
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#issuecomment-762843890
# [Codecov](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=h1) Report
> Merging [#9262](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=desc) (f0b002f) into [master](https://codecov.io/gh/apache/arrow/commit/1393188e1aa1b3d59993ce7d4ade7f7ac8570959?el=desc) (1393188) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9262 +/- ##
=======================================
Coverage 81.61% 81.61%
=======================================
Files 215 215
Lines 51867 51891 +24
=======================================
+ Hits 42329 42352 +23
- Misses 9538 9539 +1
```
| [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [rust/parquet/src/arrow/schema.rs](https://codecov.io/gh/apache/arrow/pull/9262/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9hcnJvdy9zY2hlbWEucnM=) | `91.66% <100.00%> (+0.16%)` | :arrow_up: |
| [rust/parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow/pull/9262/diff?src=pr&el=tree#diff-cnVzdC9wYXJxdWV0L3NyYy9lbmNvZGluZ3MvZW5jb2RpbmcucnM=) | `94.86% <0.00%> (-0.20%)` | :arrow_down: |
| [rust/arrow/src/array/transform/fixed\_binary.rs](https://codecov.io/gh/apache/arrow/pull/9262/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvdHJhbnNmb3JtL2ZpeGVkX2JpbmFyeS5ycw==) | `84.21% <0.00%> (+5.26%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=footer). Last update [903b41c...f0b002f](https://codecov.io/gh/apache/arrow/pull/9262?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9262: ARROW-11317: Run tests once in CI (not twice) and include prettyprint feature
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#discussion_r560545792
##########
File path: .github/workflows/rust.yml
##########
@@ -109,14 +109,14 @@ jobs:
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd rust
- cargo test
+ # run tests on all workspace members
+ cargo test --all --features=prettyprint
# test datafusion examples
cd datafusion
cargo run --example csv_sql
cargo run --example parquet_sql
cd ..
cd arrow
- cargo test
Review 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9262: ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9262:
URL: https://github.com/apache/arrow/pull/9262#issuecomment-763597193
You can now see that the pretty print tests are running:
![Uploading Screen Shot 2021-01-20 at 8.12.13 AM.png…]()
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org