-
Notifications
You must be signed in to change notification settings - Fork 173
test(rust): add regression tests for windows crates #3695
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
test(rust): add regression tests for windows crates #3695
Conversation
|
Testing here on CI. I based this PR branch on a commit before the fix so this should fail on CI with this error, After I confirm that, I'll rebase and see if CI passes. |
|
The first run actually failed because the new test surfaced a clippy faliure, |
|
CI actually passed when I expected it would fail. It turns out this is because our test command is this, It's not explicitly documented, but
@lidavidm is there any value in this, given what I just learned above? I was hoping to add a regression test as you suggested in #3693 (comment). |
|
Huh. I think there's value in doing a test with default features. I think it should run on all platforms, and can just be another step after the current tests. |
|
Yes, CI has been successful so far because optional dependencies are recognized as separate features and enabled by CI. So what I was thinking was that we would need to run the tests twice, once with arrow-adbc/.github/workflows/rust.yml Lines 240 to 243 in e2eb4b0
|
|
Thanks. I'll add another step. I'm noticing CI is already taking quite some time but we can deal with that another day. |
|
I pushed up a new step but it still passes. The exact same command that's used in the step fails locally. Looking at the raw logs, I see the clippy step pulls down the windows crates so I wonder if that's doing something unexpected? |
|
I pushed a change to test my theory but I also tested it locally and the clippy command doesn't interfere with the cargo test command so that's probably not what's going on. I'll look at this tomorrow. |
IIUC, isn't CI running on the post-merge state ( |
62f63e3 to
1750327
Compare
|
Umm... I'm a bit terrified I have been using GitHub for this long and didn't know that that's how the checkout action works... Thanks @eitsupi. I rebased off main and am testing making the crates optional. |
|
Okay that seemed to fail just fine, see https://github.com/apache/arrow-adbc/actions/runs/19275767296/job/55115083268?pr=3695. I'll revert the test change. |
1750327 to
3cf217c
Compare
Adds regression tests for the Windows-only issue found in #3693.
Changes:
driver_managercrate tests which ensure the Windows-only deps are pulled in at build/test timerust.yml) to now also run tests with default features, in addition to all features. We were previously only running the tests with all features enabled which was hiding the mistake originally caught in fix(rust/driver_manager): remove optional property for windows deps #3693. Now we run tests both ways.DriverInfostruct so the above tests pass