Skip to content

c8d: Adjust some integration-cli tests#46561

Merged
thaJeztah merged 3 commits intomoby:masterfrom
vvoland:c8d-integrationcli-skipsome
Sep 28, 2023
Merged

c8d: Adjust some integration-cli tests#46561
thaJeztah merged 3 commits intomoby:masterfrom
vvoland:c8d-integrationcli-skipsome

Conversation

@vvoland
Copy link
Contributor

@vvoland vvoland commented Sep 28, 2023

See commits

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland added status/2-code-review area/testing containerd-integration Issues and PRs related to containerd integration labels Sep 28, 2023
@vvoland vvoland added this to the 25.0.0 milestone Sep 28, 2023
Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

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

Yay, let's skip that TestPullFailsWithAlteredLayer test :D

@vvoland
Copy link
Contributor Author

vvoland commented Sep 28, 2023

Ideally, we should rewrite the TestPullFailsWithAlteredLayer in a way that would fetch a layer that's not already present, but that can wait for now.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +565 to +568
assert.Assert(c, is.Contains(out, "unexpected commit digest"))
assert.Assert(c, is.Contains(out, "expected "+manifestDigest))
} else {
assert.Assert(c, is.Contains(out, fmt.Sprintf("manifest verification failed for digest %s", manifestDigest)))
Copy link
Member

Choose a reason for hiding this comment

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

Clearly not for this PR, but I know that manifest verification failed for digest has been a good error to search for; perhaps we can wrap containerd's error to add that as prefix in the error we return 🤔

// This is the schema2 version of the test.
func (s *DockerRegistrySuite) TestPullFailsWithAlteredLayer(c *testing.T) {
testRequires(c, DaemonIsLinux)
skip.If(c, testEnv.UsingSnapshotter(), "Faked layer is already in the content store, so it won't be fetched from the repository at all.")
Copy link
Member

Choose a reason for hiding this comment

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

I'm good with skipping for now; would there still be a use to detect corrupted / manipulated data in the content-store (does containerd have anything for that?)

Comment on lines +119 to +122
expectedMsg := fmt.Sprintf("manifest for %s not found", imageReference)
if testEnv.UsingSnapshotter() {
expectedMsg = fmt.Sprintf("%s: not found", imageReference)
}
Copy link
Member

Choose a reason for hiding this comment

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

We should really ditch these tests and have a test in integration/ (checking the error-response as well as status, instead of just the string-matching).

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

Labels

area/testing containerd-integration Issues and PRs related to containerd integration status/2-code-review

Projects

Development

Successfully merging this pull request may close these issues.

3 participants