Skip to content

test(extra): cover proxy pipe cross-process access - #1448

Merged
Benoît Cortier (CBenoit) merged 1 commit into
masterfrom
dvc-proxy-split-plan
Jul 28, 2026
Merged

test(extra): cover proxy pipe cross-process access#1448
Benoît Cortier (CBenoit) merged 1 commit into
masterfrom
dvc-proxy-split-plan

Conversation

@mamoreau-devolutions

Copy link
Copy Markdown
Contributor

Summary

  • verify a DvcNamedPipeProxy Windows pipe is visible to a separately spawned process
  • require the child process to forward pipe data into the parent DVC callback

Tests

  • cargo test -p ironrdp-testsuite-extra --test integration_tests_extra dvc_pipe_proxy
  • cargo clippy -p ironrdp-testsuite-extra --test integration_tests_extra --no-deps -- -D warnings

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds Windows integration coverage proving DvcNamedPipeProxy supports cross-process pipe access and forwards data into the DVC callback.

Changes:

  • Spawns a separate test process as a named-pipe client.
  • Refactors shared proxy setup and forwarding assertions.
  • Adds the required ironrdp-svc test dependency.

Reviewed changes

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

File Description
crates/ironrdp-testsuite-extra/tests/dvc_pipe_proxy.rs Adds cross-process pipe forwarding coverage.
crates/ironrdp-testsuite-extra/Cargo.toml Adds the SVC test dependency.
Cargo.lock Records the dependency update.

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

Comment thread crates/ironrdp-dvc-pipe-proxy/src/proxy.rs Outdated
Comment thread crates/ironrdp-dvc-pipe-proxy/src/proxy.rs Outdated

@CBenoit Benoît Cortier (CBenoit) left a comment

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.

Thank you!

Some review items from Claude that I think are worth addressing:

The hardcoded child-test path is stale-prone with a misleading failure mode

dvc_pipe_proxy.rs:48 — "dvc_pipe_proxy::child_process_connects_and_forwards_windows_pipe_data"
must track both the module name in main.rs (mod dvc_pipe_proxy;) and the fn name. If
either is renamed, --exact matches zero tests; libtest then exits 0, so
assert!(status.success()) (line 54) passes, and the test instead fails 1s later at
assert_forwarded_data with "must forward pipe data to its callback" — pointing at a
forwarding bug that doesn't exist. This is the first use of the current_exe re-exec
pattern in the repo, so there's no established guard. Consider asserting the child
actually ran a test (e.g. parse --list or check the summary), or at minimum a comment
binding the string to the fn so a rename is caught in review.

child_process_… is a real test that silently no-ops in normal runs

dvc_pipe_proxy.rs:62-65 — as a #[tokio::test] it's discovered in every normal run,
where the env var is absent so it returns Ok early and shows up green. Its name reads
like a standalone assertion ("connects_and_forwards…") but in the normal suite it
asserts nothing. A name that signals its helper role (e.g. child_process_helper_…)
would make the test output honest without changing the --exact filter meaningfully.

Comment thread crates/ironrdp-testsuite-extra/tests/dvc_pipe_proxy.rs Outdated
@CBenoit Benoît Cortier (CBenoit) changed the title test(testsuite-extra): cover proxy pipe cross-process access test(extra): cover proxy pipe cross-process access Jul 14, 2026
@CBenoit
Benoît Cortier (CBenoit) enabled auto-merge (squash) July 14, 2026 03:44
Add Windows cross-process named-pipe forwarding coverage and teardown diagnostics.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
Benoît Cortier (CBenoit) merged commit 3f52e7d into master Jul 28, 2026
22 checks passed
@CBenoit
Benoît Cortier (CBenoit) deleted the dvc-proxy-split-plan branch July 28, 2026 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants