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.

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

Minor: extend ci to test each package #7940

Closed
wants to merge 6 commits into from
Closed

Conversation

Weijun-H
Copy link
Member

Which issue does this PR close?

Related #7933

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@alamb
Copy link
Contributor

alamb commented Oct 26, 2023

FYI CI is going to fail #7939

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

❤️ -- thank you @Weijun-H

@@ -65,8 +65,11 @@ jobs:
# this key equals the ones on `linux-build-lib` for re-use
key: cargo-cache-benchmark-${{ hashFiles('datafusion/**/Cargo.toml', 'benchmarks/Cargo.toml', 'datafusion-cli/Cargo.toml') }}

- name: Check workspace without default features
run: cargo check --no-default-features -p datafusion
- name: Check workspace without default features in each package tests
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really neat way of iterating over the packages

@Weijun-H
Copy link
Member Author

Draft until fix #7933

@alamb
Copy link
Contributor

alamb commented Oct 26, 2023

BTW there are a few other outstanding PRs that might help get the checks working -- #7934

@alamb
Copy link
Contributor

alamb commented Oct 26, 2023

#7933 is fixed

@github-actions github-actions bot added the core Core DataFusion crate label Oct 26, 2023
@Weijun-H
Copy link
Member Author

Weijun-H commented Oct 26, 2023

#7933 is fixed

@alamb I tried to merge #7934, it didn't solve #7933

@Weijun-H Weijun-H force-pushed the extend-ci branch 2 times, most recently from 3149b3f to 4ec8399 Compare October 26, 2023 20:22
Comment on lines +74 to +78
- name: Check tests in each package
run: |
for package in $(cargo metadata --no-deps --format-version 1 | jq -r '.packages[].name'); do
cargo check --tests --package "$package" || exit 1
done
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if we need --no-default-features to check tests 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this command verifies that cargo test --package "$package" will compile successfully, but it doesn't check if cargo test --no-default-features --package "$package" would run successfully

But I am not sure how common running cargo test --no-default-features --package "$package" is 🤔

@alamb
Copy link
Contributor

alamb commented Oct 29, 2023

This PR looks like it would be good to go if it got rebased against main -- is that correct @Weijun-H ?

@Weijun-H
Copy link
Member Author

This PR looks like it would be good to go if it got rebased against main -- is that correct @Weijun-H ?

Yes, it is.

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 Apr 24, 2024
@Weijun-H Weijun-H closed this Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate Stale PR has not had any activity for some time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants