Skip to content

test(conformance): migrate create-upload tests to testkit - #4606

Open
politerealism wants to merge 2 commits into
NVIDIA:mainfrom
politerealism:test/3954-create-upload-stories/politerealism
Open

politerealism wants to merge 2 commits into
NVIDIA:mainfrom
politerealism:test/3954-create-upload-stories/politerealism

Conversation

@politerealism

@politerealism politerealism commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Move the three sandbox create --upload tests from the legacy e2e crate into the testkit conformance suite. The behavior isn't driver-specific, so it runs against any prepared gateway. Add ExecOutput::success and Sandbox::sh to testkit so the tests don't hand-write the exit-code check and the sh -c wrapper.

Related Issue

Contributes to #3954. No separate issue: this migrates existing coverage.

Changes

  • Add ExecOutput::success(), matching CliOutput::success(), and a public command field so the failure message names the command. Add Sandbox::sh(script), which runs sh -c.
  • Add tests/suites/conformance/tests/create_upload.rs with three tests, keeping the original names so filters keep working: create_with_upload_directory_preserves_source_basename, create_with_multiple_uploads and create_with_upload_single_file.
  • Verify uploads by reading files back with Sandbox::sh and comparing exact content. The originals matched the printed output of sandbox create.
  • Check the nested file in the directory test, and that the directory isn't flattened into the destination. Check each destination separately in the multiple-uploads test.
  • Use the default upload path. The originals passed --no-git-ignore, which isn't needed outside a Git work tree and hid the default path.
  • Create the sandbox from the target's default image. The originals used the e2e test image.
  • Drop the claim that files arrive before the command runs. The CLI rejects --upload together with a command, so each test checks the files once create returns, and the module doc notes the limitation.
  • Delete e2e/rust/tests/upload_create.rs and the now-unused SandboxGuard::create_with_upload and create_with_uploads from the legacy harness.
  • Add a create-upload row to the conformance README.

main has no end-to-end coverage of sandbox create --upload outside the legacy test this PR removes. crates/openshell-cli has integration tests for the planning and error paths, but they stub the gateway and SSH transport. tests/suites/conformance/tests/file_transfer.rs covers the separate sandbox upload command. None of them check directory basenames, multiple uploads or a single-file upload on a real sandbox.

Testing

  • mise run pre-commit passes on both commits
  • cargo clippy --tests is clean in tests/suites/conformance
  • The three tests against a local Docker gateway. Not run yet.
  • Conformance in the CI lanes (test:e2e). Not run yet.
  • Not run: macOS.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (not applicable)

@copy-pr-bot

copy-pr-bot Bot commented Oct 15, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

Copy link
Copy Markdown

This pull request has had no activity for 14 days and is now marked stale.

@github-actions github-actions Bot added the state:stale Inactive item at risk of automatic closure. label Oct 30, 2026
elezar and others added 2 commits November 3, 2026 18:03
Give `ExecOutput` the same `success()` check that `CliOutput` has, and
record the command so the failure message names it. Add `Sandbox::sh` to
run a shell script with `sh -c`.

Suites hand-write the exit-code check and the `sh -c` wrapper after every
`exec`; these let them drop that boilerplate.

Signed-off-by: Evan Lezar <elezar@nvidia.com>
Move the three `sandbox create --upload` tests from the legacy e2e crate
into the testkit conformance suite. The behavior is not driver-specific,
so it runs against any prepared gateway.

Test names are unchanged so filters keep working:
- create_with_upload_directory_preserves_source_basename
- create_with_multiple_uploads
- create_with_upload_single_file

Changes from the originals:
- Verify uploads by reading the files back with `sandbox exec` and
  comparing exact content, instead of matching the printed output of
  `sandbox create`.
- Check the nested file in the directory test, and that the directory
  is not flattened into the destination.
- Check each destination in the multiple-uploads test separately.
- Use the default upload path. The originals passed `--no-git-ignore`,
  which is unnecessary outside a Git work tree and hid the default path.
- Create the sandbox from the target's default image.
- Drop the claim that files arrive before the command runs. The CLI
  rejects `--upload` together with a command, so each test checks the
  files once `create` returns and documents the limitation.

Remove the now-unused `SandboxGuard::create_with_upload` and
`create_with_uploads` from the legacy harness, and the old test file.

Contributes to NVIDIA#3954

Co-authored-by: Evan Lezar <elezar@nvidia.com>
Signed-off-by: politerealism <burdcat17@gmail.com>
@elezar
elezar force-pushed the test/3954-create-upload-stories/politerealism branch from 7d181e5 to 613b95b Compare November 3, 2026 17:11
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Maintainer list change

Gains approval rights:

Confirm every change is intended. Anyone listed here can single-handedly satisfy OpenShell / Maintainer Approval.

@elezar
elezar changed the base branch from codex/4119-suite-layout to main November 3, 2026 17:13
@elezar elezar changed the title test(conformance): migrate create-upload tests to file_transfer stories test(conformance): migrate create-upload tests to testkit Nov 3, 2026
@elezar

elezar commented Nov 3, 2026

Copy link
Copy Markdown
Member

/ok to test 613b95b

@elezar elezar added the test:e2e Requires end-to-end coverage label Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

Label test:e2e applied for 613b95b. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway, sandbox, and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state:stale Inactive item at risk of automatic closure. test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants