Skip to content

ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage #9262

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

Closed
alamb wants to merge 2 commits into from
Closed

ARROW-11317: [Rust] Include the prettyprint feature in CI Coverage #9262

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd4d5bd
ARROW-11317: Run tests once in CI (not twice) and include prettyprint…
alamb Jan 19, 2021
063a7c8
keep second CI run
alamb Jan 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/rust.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,16 @@ jobs:
export CARGO_HOME="/github/home/.cargo"
export CARGO_TARGET_DIR="/github/home/target"
cd rust
# run tests on all workspace members with default feature list
cargo test
# test datafusion examples
cd datafusion
cargo run --example csv_sql
cargo run --example parquet_sql
cd ..
cd arrow
cargo test

@alamb alamb Jan 19, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@paddyhoran paddyhoran Jan 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@alamb alamb Jan 19, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@alamb alamb Jan 20, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@nevi-me nevi-me Jan 22, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@alamb alamb Jan 22, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It has been resolved (though the resolution was to keep running the tests for arrow twice)

@jorgecarleitao jorgecarleitao Jan 22, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 ^_^

@alamb alamb Jan 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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)

# re-run tests on arrow workspace with additional features
cargo test --features=prettyprint
cargo run --example builders
cargo run --example dynamic_types
cargo run --example read_csv
Expand Down