Skip to content

Conversation

@amoeba
Copy link
Member

@amoeba amoeba commented Nov 10, 2025

Adds regression tests for the Windows-only issue found in #3693.

Changes:

  • Adds to basic regression tests to the driver_manager crate tests which ensure the Windows-only deps are pulled in at build/test time
  • Changes the Rust workflow (rust.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.
  • Minor: Adds the Debug trait to the DriverInfo struct so the above tests pass

@amoeba
Copy link
Member Author

amoeba commented Nov 10, 2025

Testing here on CI. I based this PR branch on a commit before the fix so this should fail on CI with this error,

error[E0432]: unresolved import `windows_sys`
    --> driver_manager\src\lib.rs:1755:9
     |
1755 |     use windows_sys as windows;
     |         ^^^^^^^^^^^^^^^^^^^^^^ no external crate `windows_sys`

error[E0433]: failed to resolve: use of unresolved module or unlinked crate `windows_registry`
   --> driver_manager\src\lib.rs:554:17
    |
554 |                 windows_registry::CURRENT_USER,
    |                 ^^^^^^^^^^^^^^^^ use of unresolved module or unlinked crate `windows_registry`
    |
    = help: if you wanted to use a crate named `windows_registry`, use `cargo add windows_registry` to add it to your `Cargo.toml`

error[E0433]: failed to resolve: use of unresolved module or unlinked crate `windows_registry`
   --> driver_manager\src\lib.rs:573:17
    |
573 |                 windows_registry::LOCAL_MACHINE,
    |                 ^^^^^^^^^^^^^^^^ use of unresolved module or unlinked crate `windows_registry`
    |
    = help: if you wanted to use a crate named `windows_registry`, use `cargo add windows_registry` to add it to your `Cargo.toml`

error[E0433]: failed to resolve: use of unresolved module or unlinked crate `windows_registry`
   --> driver_manager\src\lib.rs:677:12
    |
677 |     root: &windows_registry::Key,
    |            ^^^^^^^^^^^^^^^^ use of unresolved module or unlinked crate `windows_registry`
    |
    = help: if you wanted to use a crate named `windows_registry`, use `cargo add windows_registry` to add it to your `Cargo.toml`

After I confirm that, I'll rebase and see if CI passes.

@amoeba
Copy link
Member Author

amoeba commented Nov 10, 2025

The first run actually failed because the new test surfaced a clippy faliure,

error[E0277]: `DriverInfo` doesn't implement `std::fmt::Debug`
    --> driver_manager\src\lib.rs:2040:27
     |
2040 |         assert_eq!(result.unwrap_err().status, Status::NotFound);
     |                           ^^^^^^^^^^ the trait `std::fmt::Debug` is not implemented for `DriverInfo`
     |
     = note: add `#[derive(Debug)]` to `DriverInfo` or manually `impl std::fmt::Debug for DriverInfo`
note: required by a bound in `std::result::Result::<T, E>::unwrap_err`
    --> /rustc/f8297e351a40c1439a467bbbb6879088047f50b3\library\core\src\result.rs:1317:5
help: consider annotating `DriverInfo` with `#[derive(Debug)]`

https://github.com/apache/arrow-adbc/actions/runs/19241306258/job/55004416512?pr=3695

@amoeba
Copy link
Member Author

amoeba commented Nov 10, 2025

CI actually passed when I expected it would fail. It turns out this is because our test command is this,

cargo test --all-targets --all-features --workspace  --exclude adbc_snowflake

It's not explicitly documented, but --all-features seems to enable all optional crates too, even those not included in a feature. When I test this manually on my Windows machine,

  • cargo build fails
  • cargo build --all-features succeeds

@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).

@lidavidm
Copy link
Member

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.

@eitsupi
Copy link
Contributor

eitsupi commented Nov 11, 2025

Yes, CI has been successful so far because optional dependencies are recognized as separate features and enabled by CI.
#3693 (review)

So what I was thinking was that we would need to run the tests twice, once with --all-features and once without.
(In fact, we are already running multiple tests for different Arrow versions.)

# If the lock file was updated, run the tests except for adbc_datafusion
if ! git diff --quiet Cargo.lock; then
cargo test --all-targets --all-features --workspace --exclude adbc_datafusion
fi

@amoeba
Copy link
Member Author

amoeba commented Nov 11, 2025

Thanks. I'll add another step. I'm noticing CI is already taking quite some time but we can deal with that another day.

@amoeba
Copy link
Member Author

amoeba commented Nov 11, 2025

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?

@amoeba
Copy link
Member Author

amoeba commented Nov 11, 2025

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.

@eitsupi
Copy link
Contributor

eitsupi commented Nov 11, 2025

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?

IIUC, isn't CI running on the post-merge state (pull/3695/merge here, where the Windows bug has already been fixed)?

   "C:\Program Files\Git\bin\git.exe" checkout --progress --force refs/remotes/pull/3695/merge
  Note: switching to 'refs/remotes/pull/3695/merge'.
  
  You are in 'detached HEAD' state. You can look around, make experimental
  changes and commit them, and you can discard any commits you make in this
  state without impacting any branches by switching back to a branch.
  
  If you want to create a new branch to retain commits you create, you may
  do so (now or later) by using -c with the switch command. Example:
  
    git switch -c <new-branch-name>
  
  Or undo this operation with:
  
    git switch -
  
  Turn off this advice by setting config variable advice.detachedHead to false
  
  HEAD is now at 1cc084125 Merge 62f63e316cfe648dc39355bcfedc6f748a28db34 into 34985ff324dbe923369d9585a2d68186e573a0e1

https://github.com/apache/arrow-adbc/actions/runs/19253604196/job/55043462638?pr=3695#step:2:179

@amoeba amoeba force-pushed the fix/rust-regression-test-for-windows-crates branch from 62f63e3 to 1750327 Compare November 11, 2025 19:02
@amoeba
Copy link
Member Author

amoeba commented Nov 11, 2025

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.

@amoeba
Copy link
Member Author

amoeba commented Nov 11, 2025

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.

@amoeba amoeba force-pushed the fix/rust-regression-test-for-windows-crates branch from 1750327 to 3cf217c Compare November 11, 2025 19:15
@amoeba amoeba marked this pull request as ready for review November 11, 2025 19:15
@github-actions github-actions bot modified the milestone: ADBC Libraries 22 Nov 11, 2025
@lidavidm lidavidm changed the title fix(rust): add regression tests for windows crates test(rust): add regression tests for windows crates Nov 12, 2025
@amoeba amoeba merged commit 7271f3c into apache:main Nov 12, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants