Skip to content

[wagon-3.x] Revive the embedded SSH tests and run them in CI - #904

Merged
slachiewicz merged 1 commit into
apache:wagon-3.xfrom
slachiewicz:ssh-tests-fix
Aug 8, 2026
Merged

[wagon-3.x] Revive the embedded SSH tests and run them in CI#904
slachiewicz merged 1 commit into
apache:wagon-3.xfrom
slachiewicz:ssh-tests-fix

Conversation

@slachiewicz

Copy link
Copy Markdown
Member

The embedded SSH tests have not run in over a decade. This gets the embedded half of them green and runs it in CI.

Why the suite could not start

PlexusTestCase uses plexus-container-default, which reads META-INF/plexus/components.xml. plexus-interactivity-api stopped shipping that descriptor in 1.3 and now ships only a sisu index — I checked the jars: 1.1 has the plexus descriptor and no sisu index, 1.5.1 the reverse. Maven itself runs on sisu, so nothing is wrong in production; only PlexusTestCase is affected.

The ComponentLookupException on the scp role hint hid five levels of cause. The bottom is a missing Prompter, which ConsoleInteractiveUserInfo and PrompterUIKeyboardInteractive both require and AbstractJschWagon needs both of. All 94 errors had that same bottom cause. It arrived with the 1.1 → 1.3 bump in 42ee3769.

Writing a descriptor for the real DefaultPrompter does not work: it takes its collaborators through a constructor into final fields, and the container's builder only does no-arg construction plus field injection. So the tests get their own Prompter, in test scope, which throws rather than blocking on stdin.

It was three faults, not one

The Prompter alone took 94 errors to 87.

  • ShellCommand flushed the output stream after ExitCallback.onExit() had closed the channel. The server logged SshChannelClosedException, the session went down, and the next command in it died. Removing that one stray flush() took the embedded tests from 19 errors to 6.
  • getExpectedLastModifiedOnGet compared at millisecond precision while the scp T header carries whole seconds, so the mock transfer event never matched and the progress assertion saw 0 bytes. Truncated to seconds.
  • The embedded server registered no SFTP subsystem, and OpenSSH 9+ drives scp over SFTP, so every shelled-out scp exited 255.

One production change, called out for review

ScpWagon.fillInputData mapped only scp ack code 1 to ResourceDoesNotExistException; MINA sshd reports a missing file with code 2. Without this, four tests stay red.

While there: line.indexOf("no such file or directory") != 1 is a typo for != -1, which makes every code-1 error a ResourceDoesNotExistException. I deliberately preserved that behaviour rather than quietly tightening it, so only code 2 is genuinely new. Worth fixing separately, with its own thought about what it changes.

Counts

before after
wagon-ssh, -Dssh-tests -Dssh-embedded=true 42 tests, 24 errors 42 tests, 0 failures
wagon-ssh, -Dssh-tests alone 112 tests, 94 errors 112 tests, 8 errors + 2 failures
full reactor with both switches BUILD SUCCESS
affected modules, no switches BUILD SUCCESS

No test was disabled or deleted.

Why bare -Dssh-tests still cannot be green

By design of those tests, not by breakage. ScpWagonTest, SshCommandExecutorTest and KnownHostsProviderTest expect a real sshd on localhost:22 and the developer's own account. SftpWagonTest and ScpWagonWithSshPrivateKeySearchTest connect to scp://localhost:0/, broken since 35ff4024 removed getTestRepositoryPort(). Only Embedded*Test uses the embedded server, which is exactly what the existing ssh-embedded profile selects — hence the two-property combination in CI.

CI

maven-args: '-D"invoker.streamLogsOnFailures" -Dssh-tests -Dssh-embedded=true'

Two pom changes make that safe, both checked empirically rather than assumed. Surefire <excludes> from two active profiles override rather than merge, last declared winning — verified by running -Dssh-embedded=true without -Dssh-tests and watching the Embedded tests run anyway. On Windows that would have let ssh-embedded beat windauze and run the /bin/sh-dependent tests, so ssh-embedded now also requires a non-Windows OS. And wagon-ssh-external's own ssh-embedded profile would have run a test that is not green, so it is excluded there with a comment; that module runs no ssh tests today, so nothing is lost.

If you would rather not carry a switch at all, dropping **/Embedded*Test.* from no-ssh-tests gives identical coverage with no workflow change.

Caveats

Only exercised on JDK 21; the CI matrix includes JDK 8, and these tests are new to CI on every version. wagon-ssh-external's EmbeddedScpExternalWagonWithKeyTest still fails on path quoting — ScpExternalWagon backslash-escapes spaces for the legacy remote-shell protocol, which SFTP-mode scp passes through verbatim. That needs a decision about -O versus changed quoting, and the answer depends on the host's OpenSSH version, so it is left alone here.

Why this matters beyond itself

This is the only harness that runs the SSH provider against a real server. It is the precursor to #902 and #903: with it in place, the JSch swap those PRs make is verified rather than asserted — and it immediately earned that, catching a NoClassDefFoundError that would have stopped wagon-ssh loading at all.

The -Dssh-tests suite has been dead for years. Three separate faults kept
the tests that use the embedded Apache MINA sshd from passing:

* plexus-interactivity-api stopped shipping META-INF/plexus/components.xml
  in 1.3, so plexus-container-default - which is what PlexusTestCase runs -
  could no longer see a Prompter, and every lookup of a jsch Wagon failed
  because ConsoleInteractiveUserInfo and PrompterUIKeyboardInteractive both
  require one. Supply a Prompter for the tests.
* ShellCommand flushed the channel's output stream after calling
  ExitCallback.onExit, which closes it. The resulting
  SshChannelClosedException tore down the session, so the command after
  every executeCommand failed with "session is down".
* The expected modification time on a get was compared at millisecond
  precision, but the scp "T" header carries whole seconds.

Also register the SFTP subsystem on the embedded server: OpenSSH 9 and
later drive scp over SFTP, so wagon-ssh-external's tests, which shell out
to the host's scp, could not connect at all.

ScpWagon now maps a missing file to ResourceDoesNotExistException when the
server reports it with the scp fatal-error code (2) and says so in the
message, not only with the warning code (1) that OpenSSH uses.

CI passes -Dssh-tests -Dssh-embedded=true, which runs the 42 embedded jsch
tests. The rest of the suite needs an sshd on localhost:22 and the
developer's own account there, and stays excluded.

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

This PR revives the embedded SSH provider test suite (primarily the embedded Apache MINA sshd-backed tests) and ensures it runs in CI by restoring Plexus wiring needed for PlexusTestCase and addressing several embedded-server/test-harness incompatibilities with modern OpenSSH/scp behavior.

Changes:

  • Add a test-scope Plexus component descriptor and a non-interactive Prompter implementation so PlexusTestCase can construct SSH wagons again.
  • Fix embedded SSH test harness behavior (SFTP subsystem for OpenSSH 9+ scp-over-SFTP, command execution flush ordering, and scp timestamp precision expectations).
  • Update CI workflow and Maven profiles to run only the embedded SSH tests safely (including avoiding Windows activation).

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
.github/workflows/maven-verify.yml Runs embedded SSH tests in CI via -Dssh-tests -Dssh-embedded=true.
wagon-providers/wagon-ssh/pom.xml Adjusts ssh-embedded profile activation/excludes to avoid unintended execution (esp. on Windows).
wagon-providers/wagon-ssh-external/pom.xml Excludes a currently non-deterministic embedded scp test from CI due to OpenSSH version-dependent behavior.
wagon-providers/wagon-ssh/src/main/java/.../ScpWagon.java Treats MINA sshd’s scp “missing file” ack behavior as ResourceDoesNotExistException.
wagon-providers/wagon-ssh/src/test/java/.../TestPrompter.java Adds a test-only Prompter that throws on prompts to keep tests non-interactive.
wagon-providers/wagon-ssh/src/test/resources/META-INF/plexus/components.xml Wires TestPrompter into the Plexus container for PlexusTestCase.
wagon-providers/wagon-ssh-common-test/src/main/java/.../SshServerEmbedded.java Enables SFTP subsystem to support OpenSSH 9+ scp-over-SFTP.
wagon-providers/wagon-ssh-common-test/src/main/java/.../ShellCommand.java Avoids flushing after the callback closes the channel (stability for multi-command sessions).
wagon-providers/wagon-ssh-common-test/src/main/java/.../AbstractEmbeddedScpWagonTest.java Truncates expected mtime to whole seconds to match scp protocol precision.
wagon-providers/wagon-ssh-common-test/src/main/java/.../AbstractEmbeddedScpWagonWithKeyTest.java Same mtime precision adjustment for key-based embedded scp tests.
Suppressed comments (1)

wagon-providers/wagon-ssh/src/main/java/org/apache/maven/wagon/providers/ssh/jsch/ScpWagon.java:234

  • This comment describes scp ack code 1 as a "warning". Regardless of server-specific behavior, the code path throws a ResourceDoesNotExistException, so calling it a warning/recoverable is misleading. Please reword the comment to describe it as an error code and keep the rest of the rationale unchanged.
                // OpenSSH reports a missing file with the scp "warning" code (1); other servers - Apache MINA
                // sshd, which the tests run against - report it with the "fatal error" code (2), so fall back on
                // the message for those

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +58 to +62
/** scp acknowledgement byte for a recoverable error, what OpenSSH sends for a missing file */
private static final int SCP_WARNING = 1;

/** scp acknowledgement byte for a fatal error */
private static final int SCP_ERROR = 2;
@slachiewicz
slachiewicz merged commit 4959538 into apache:wagon-3.x Aug 8, 2026
8 checks passed
@github-actions github-actions Bot added this to the 3.5.4 milestone Aug 8, 2026
slachiewicz pushed a commit that referenced this pull request Aug 8, 2026
The -Dssh-tests suite has been dead for years. Three faults kept the tests that
use the embedded Apache MINA sshd from passing:

* ShellCommand flushed the channel's output stream after calling
  ExitCallback.onExit, which closes it. The resulting SshChannelClosedException
  tore down the session, so the command after every executeCommand failed with
  "session is down".
* The expected modification time on a get was compared at millisecond precision,
  but the scp "T" header carries whole seconds.
* The SFTP subsystem was not registered on the embedded server. OpenSSH 9 and
  later drive scp over SFTP, so wagon-ssh-external's tests, which shell out to
  the host's scp, could not connect at all.

ScpWagon now maps a missing file to ResourceDoesNotExistException when the server
reports it with the scp fatal-error code (2) and says so in the message, not only
with the warning code (1) that OpenSSH uses.

CI passes -Dssh-tests -Dssh-embedded=true, which runs the 42 embedded jsch tests.

This is the 3.x change from #904 without its fourth part. There, a TestPrompter
and a test component descriptor had to be supplied by hand, because
plexus-interactivity-api stopped shipping META-INF/plexus/components.xml in 1.3
and plexus-container-default could see neither the sisu index nor DefaultPrompter's
constructor injection. Since #911 the tests run on the Sisu shim, which reads that
index and satisfies the constructor, so the real DefaultPrompter resolves and the
workaround is not needed here. Verified: 42 tests, 0 failures without it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Pull requests that change the build process maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants