Skip to content

Test freedesktop trash implementation in container#144

Merged
Byron merged 18 commits into
Byron:masterfrom
null-dev:nd/canonicalize-trash-tests
Apr 19, 2026
Merged

Test freedesktop trash implementation in container#144
Byron merged 18 commits into
Byron:masterfrom
null-dev:nd/canonicalize-trash-tests

Conversation

@null-dev
Copy link
Copy Markdown
Contributor

@null-dev null-dev commented Apr 1, 2026

This PR adds tests that boot linux containers to test the freedesktop trash logic. I had Claude Code write these tests to validate my changes in #143.

There is some hacking in the code to get the containers working so feel free to close this PR if you don't feel these tests are not worth the extra code.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds Linux-only integration tests that validate the crate’s freedesktop trash behavior under complex mount/symlink setups by running the logic inside privileged Docker containers, using a small helper CLI binary to drive trash::delete.

Changes:

  • Added container-based freedesktop trash integration tests (Linux-only) covering directories, files, symlinks, mounts, and mount/symlink permutations.
  • Added a minimal trash-helper CLI binary used by the container tests.
  • Added Linux-only dev-dependencies for testcontainers and tokio to support the async container tests.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.

File Description
tests/freedesktop_tests.rs New Docker-backed integration tests for freedesktop trash behavior across mounts/symlinks.
src/bin/trash-helper.rs Minimal CLI wrapper to invoke trash::delete from inside the container.
Cargo.toml Adds Linux-only dev dependencies required to compile/run the new container tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Cargo.toml Outdated
Comment on lines +12 to +13

#![cfg(target_os = "linux")]
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

These tests require a working Docker daemon and a privileged container that can perform mounts. The repo’s CI runs tests via cross test (see .github/workflows/rust.yml), which executes inside a Docker container and typically cannot start sibling privileged containers; this will make cargo test fail in CI and for many dev setups. Consider gating these behind a feature/env var and/or marking them #[ignore] by default, with a separate CI job that explicitly enables them when Docker is available.

Suggested change
#![cfg(target_os = "linux")]
// - Explicit opt-in when compiling tests, e.g.
// `RUSTFLAGS="--cfg docker_privileged_tests" cargo test`.
#![cfg(all(target_os = "linux", docker_privileged_tests))]

Copilot uses AI. Check for mistakes.
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.

Fixed by gating the tests behind the _container-tests feature. This feature is an internal-use only feature and will not appear in Rustdoc because it is prefixed with an underscore.

I used a feature, and not a cfg flag because we only want to build the trash-test-helper binary if we are on Linux and running the container-based tests. If we try to build the binary on netbsd, it results in an error and breaks CI. AFAIK features are the only way to conditionally build a binary. I don't think it's possible to do the same using a cfg flag.

Comment thread tests/freedesktop_tests.rs Outdated
Comment thread tests/freedesktop_tests.rs Outdated
Comment thread tests/freedesktop_tests.rs
@Byron
Copy link
Copy Markdown
Owner

Byron commented Apr 2, 2026

@null-dev Thanks! I think this can be merged once the auto-review comments are resolved and CI is happy.

[target.'cfg(target_os = "linux)'.dev-dependencies]

It's amazing this ever worked for you. For me this is a flag, but I give it the benefit of doubt.

@Byron
Copy link
Copy Markdown
Owner

Byron commented Apr 6, 2026

I just merged #143 and think this PR should be merged as well, possibly with fixes, before I cut a new release.

@null-dev
Copy link
Copy Markdown
Contributor Author

null-dev commented Apr 6, 2026

I just merged #143 and think this PR should be merged as well, possibly with fixes, before I cut a new release.

Yep, trying to get CI working on this as we speak. I'm running into some difficulties as your CI runs all the tests in a docker container, and the tests I added spawn their own docker containers. Additionally, Github actions runs CI jobs in docker containers. So we are running docker-in-docker-in-docker.

Will let you know once I get it working.

@vp-oryn vp-oryn force-pushed the nd/canonicalize-trash-tests branch 2 times, most recently from 9fac0cb to 353d2a5 Compare April 6, 2026 05:38
@Byron
Copy link
Copy Markdown
Owner

Byron commented Apr 6, 2026

Great to hear!
You can probably create your own job or workflow if that helps with the docker-in-docker part.

@vp-oryn vp-oryn force-pushed the nd/canonicalize-trash-tests branch 2 times, most recently from 748dfeb to e93f105 Compare April 6, 2026 07:23
null-dev added 2 commits April 6, 2026 03:28
We need to use a feature because it is the only way to conditionally build a binary


Update freedesktop test run instructions
@vp-oryn vp-oryn force-pushed the nd/canonicalize-trash-tests branch from e93f105 to 7848be9 Compare April 6, 2026 07:28
@null-dev
Copy link
Copy Markdown
Contributor Author

null-dev commented Apr 6, 2026

Great to hear! You can probably create your own job or workflow if that helps with the docker-in-docker part.

CI should be good now. I wanted to avoid making a separate job because that would cause the library to be compiled twice, adding to CI time. But I can still use a separate job if you feel the current solution is too hacky.

Also, FYI, I had to add a new feature (that is only used internally), see: #144 (comment) . This resolves a separate issue: CI was failing on netbsd because the netbsd job calls cargo build. It looks like if cargo build outputs any binary at all on netbsd, CI will fail. I don't fully understand the netbsd build so maybe there is a better way to fix this. I believe an alternative solution is to change the netbsd step to call cargo build --lib so cargo won't attempt to build the binary?

@vp-oryn vp-oryn force-pushed the nd/canonicalize-trash-tests branch from e6573d1 to df55dc0 Compare April 11, 2026 21:28
@null-dev
Copy link
Copy Markdown
Contributor Author

null-dev commented Apr 11, 2026

Alright, changing the netbsd CI to call cargo build --lib actually works, so I was able to remove the feature.

We can use autobins to handle this now that it doesn't reference the feature
Copy link
Copy Markdown
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

Just one small change that might help with --lib as well, and it could be ready to go.
As a note: each time this is force-pushed I have to re-review everything.

Comment thread examples/trash.rs
/// Usage: trash-test-helper delete <path>
///
/// Exits 0 on success, 1 on trash error, 2 on bad arguments.
fn main() {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can we call this trash and put it into examples? I'd like to avoid src/bin while it's used just for testing.

Copy link
Copy Markdown
Contributor Author

@null-dev null-dev Apr 14, 2026

Choose a reason for hiding this comment

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

Ah good idea, done! Just be aware that the CARGO_BIN_EXE_ trick Copilot mentioned here doesn't work with example bins so I had to go change that logic back to the manual detection approach.

Comment thread .github/workflows/rust.yml Outdated
Copy link
Copy Markdown
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Looks great, and I feel like I learned something about test setups!

@Byron Byron merged commit c21e99e into Byron:master Apr 19, 2026
4 checks passed
@null-dev null-dev deleted the nd/canonicalize-trash-tests branch April 19, 2026 17:37
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.

3 participants