Skip to content

pkg/copy: add tests for ParseSourceAndDestination#28908

Open
ROKUMATE wants to merge 1 commit into
podman-container-tools:mainfrom
ROKUMATE:main
Open

pkg/copy: add tests for ParseSourceAndDestination#28908
ROKUMATE wants to merge 1 commit into
podman-container-tools:mainfrom
ROKUMATE:main

Conversation

@ROKUMATE

@ROKUMATE ROKUMATE commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Fixes: #28907

What

Add unit tests for ParseSourceAndDestination in pkg/copy, the parser behind
podman cp arguments ([nameOrID:]path). It had no test coverage.

Why

The parsing rules are easy to regress: a leading .// keeps a colon as part
of the path (not a container ref), absolute paths never denote a container, and
both paths must be non-empty. Tests lock this behavior in.

Details

Table-driven tests in pkg/copy/parse_test.go covering:

  • container source -> local dest, and the reverse
  • colon inside a path that starts with . or /
  • a relative path with no colon (no container)
  • error cases: missing source path, missing dest path, both empty

No production code changes. go test ./pkg/copy/ passes in local run.

@packit-as-a-service

Copy link
Copy Markdown

tmt tests failed for commit e0a8980. @lsm5, @psss, @thrix please check.

@ROKUMATE

Copy link
Copy Markdown
Contributor Author

@Honny1 @danishprakash can you help me with the review of this pr as well
i am not sure why the ci are failing here as the tests i wrote and ci issue are not related acc. to me
also the test passed on local run

@Honny1 Honny1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please add Fixes: #issueNumber to the commit msg? Otherwise LGTM.

@Honny1

Honny1 commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Packit is unstable.

@ROKUMATE

Copy link
Copy Markdown
Contributor Author

ohkk will do it thanks for review

@packit-as-a-service

Copy link
Copy Markdown

tmt tests failed for commit ba72933. @lsm5, @psss, @thrix please check.

@Honny1

Honny1 commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

/packit rebuild-failed

@ROKUMATE

Copy link
Copy Markdown
Contributor Author

@danishprakash, @Honny1 the issue is up from a week if any points i have missed solving it do point it out ... so i will solve it or if the pr is invalid for current time then i will try solving other bugs ...

@Honny1 Honny1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm missing some edge cases:

  • Multiple colons: ctr:/path/with:colon should split only on the first colon
  • No container on either side: two local paths like /src, /dst (function allows it despite docstring)
  • Container on both sides: a:/x, b:/y (function allows it, no validation)
  • Bare filename: myfile.txt with no colon, dot, or slash prefix
  • Whitespace in path: ctr:/path with spaces/file
  • Source only empty, destination valid: "", /dst (distinct from "both empty")

Otherwise LGTM

Fixes: podman-container-tools#28907

Signed-off-by: ROKUMATE <rohitkumawat0110@gmail.com>
@ROKUMATE

Copy link
Copy Markdown
Contributor Author

Resolved the additional test cases that you mentioned sir 👍🏼

@Honny1 Honny1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@Honny1

Honny1 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

PTAL @podman-container-tools/podman-maintainers @podman-container-tools/podman-reviewers

@inknos

inknos commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

LGTM, something's up with the tests but seems unrelated

Edit: my bad, Matt said it right

@mheon

mheon commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

I think the test failures are legitimate. I bet you it's because the tests use forward-slash and, on Windows, our parsing wants backslashes?

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.

Test coverage for pkg/copy ParseSourceAndDestination

4 participants