CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Drop gotest.tools #6762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop gotest.tools #6762
Conversation
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I left suggestions where I think the use of Testify could be more idiomatic, but I don't think any of those should be blocking.
defer unlock("testref") | ||
|
||
err = tryLock("testref") | ||
assert.ErrorContains(t, err, "ref testref locked for ") | ||
require.NotNil(t, err) | ||
assert.Contains(t, err.Error(), "ref testref locked for ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.Contains(t, err.Error(), "ref testref locked for ") | |
assert.ErrorContains(t, err, "ref testref locked for ") |
"%d > %d", layer2Usage.Size, sizeBytes) | ||
} | ||
|
||
func TestMkfsExt4(t *testing.T) { | ||
ctx := context.Background() | ||
// We test the default setting which is lazy init is disabled | ||
err := mkfs(ctx, "ext4", "nodiscard,lazy_itable_init=0,lazy_journal_init=0", "") | ||
assert.ErrorContains(t, err, `mkfs.ext4 couldn't initialize ""`) | ||
assert.Contains(t, err.Error(), `mkfs.ext4 couldn't initialize ""`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.Contains(t, err.Error(), `mkfs.ext4 couldn't initialize ""`) | |
assert.ErrorContains(t, err, `mkfs.ext4 couldn't initialize ""`) |
assert.NotNil(t, err) | ||
if err != nil { | ||
assert.Contains(t, err.Error(), "remove") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be simplified
assert.NotNil(t, err) | |
if err != nil { | |
assert.Contains(t, err.Error(), "remove") | |
} | |
assert.ErrorContains(t, err, "remove") |
I've addressed the majority of suggestions. |
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Currently we rely on two different helper libraries for testing:
gotest.tools/v3/assert
github.com/stretchr/testify/assert
Both offer more or less same set of APIs, so there is no reason to keep both.
Looks like
testify
got better adoption accross containerd's codebase.So this PR migrates tests to
testify
and dropsgotest.tools/v3/assert
dependency.