Skip to content

Add unit test for plugin#5666

Merged
dmcgowan merged 1 commit intocontainerd:mainfrom
wllenyj:plugin_test
Jul 9, 2021
Merged

Add unit test for plugin#5666
dmcgowan merged 1 commit intocontainerd:mainfrom
wllenyj:plugin_test

Conversation

@wllenyj
Copy link
Copy Markdown
Contributor

@wllenyj wllenyj commented Jun 27, 2021 •
edited
Loading

Signed-off-by: wanglei [email protected]

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Jun 27, 2021

Hi @wllenyj. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jun 27, 2021

Build succeeded.

estesp
estesp reviewed Jun 28, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think there is a typo in one of the "type" entries; did you run make test to validate your test? my bad

Comment thread plugin/plugin_test.go Outdated
Copy link
Copy Markdown
Member

@estesp estesp Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like a typo? s/GCPlugin/GRPCPlugin/

Edit: this is a "GC" plugin.. oops; reading too fast :)

Copy link
Copy Markdown
Contributor Author

@wllenyj wllenyj Jun 29, 2021

Choose a reason for hiding this comment

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

thanks for review :)

@mxpv
Copy link
Copy Markdown
Member

mxpv commented Jun 28, 2021

@wllenyj looks like linter is not happy:

Error: File is not `gofmt`-ed with `-s` (gofmt)
  Error: don't use underscores in Go names; var `expect_uri` should be `expectURI` (golint)
  Error: don't use underscores in Go names; func parameter expect_uri should be expectURI (golint)
  Error: don't use underscores in Go names; struct field expected_uri should be expectedURI (golint)

PTAL?

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jun 29, 2021

Build succeeded.

@wllenyj
Copy link
Copy Markdown
Contributor Author

wllenyj commented Jun 29, 2021

@wllenyj looks like linter is not happy:

Error: File is not `gofmt`-ed with `-s` (gofmt)
  Error: don't use underscores in Go names; var `expect_uri` should be `expectURI` (golint)
  Error: don't use underscores in Go names; func parameter expect_uri should be expectURI (golint)
  Error: don't use underscores in Go names; struct field expected_uri should be expectedURI (golint)

PTAL?

@mxpv fixed

ktock
ktock reviewed Jun 29, 2021
View reviewed changes
Comment thread plugin/plugin_test.go Outdated
Comment thread plugin/plugin_test.go Outdated
Copy link
Copy Markdown
Member

@ktock ktock Jun 29, 2021

Choose a reason for hiding this comment

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

ditto

Copy link
Copy Markdown
Contributor Author

@wllenyj wllenyj Jun 29, 2021

Choose a reason for hiding this comment

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

added

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jun 29, 2021

Build succeeded.

ktock
ktock reviewed Jun 29, 2021
View reviewed changes
Comment thread plugin/plugin_test.go Outdated
Comment on lines 42 to 43
Copy link
Copy Markdown
Member

@ktock ktock Jun 29, 2021

Choose a reason for hiding this comment

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

Why is this test needed? If the purpose is to test Graph, isn't TestPluginGraph just enough?

Copy link
Copy Markdown
Contributor Author

@wllenyj wllenyj Jun 29, 2021

Choose a reason for hiding this comment

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

TestContainerdPlugin tries to simulate the use of containerd as much as possible, Graph is the constructed test case. I think it is clearer to separate them.

Comment thread plugin/plugin_test.go Outdated
Comment on lines 25 to 33
Copy link
Copy Markdown
Member

@ktock ktock Jun 29, 2021

Choose a reason for hiding this comment

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

Do they need to be public functions?

Copy link
Copy Markdown
Contributor Author

@wllenyj wllenyj Jun 29, 2021

Choose a reason for hiding this comment

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

fixed

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jun 29, 2021

Build succeeded.

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Jul 1, 2021

Code looks good, can you use your real name on the sign off though rather than Github username

@dmcgowan dmcgowan added the status/needs-update Awaiting contributor update label Jul 1, 2021
Add unit test for plugin
3a0b9ec 
Signed-off-by: wanglei <[email protected]>
@wllenyj
Copy link
Copy Markdown
Contributor Author

wllenyj commented Jul 2, 2021

Code looks good, can you use your real name on the sign off though rather than Github username

Done :)

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jul 2, 2021

Build succeeded.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jul 2, 2021

Build succeeded.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jul 2, 2021

Build succeeded.

@wllenyj
Copy link
Copy Markdown
Contributor Author

wllenyj commented Jul 5, 2021

/retest

what's going on? Can anyone give me some suggestions?

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Jul 5, 2021

@wllenyj: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

what's going on? Can anyone give me some suggestions?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kzys
kzys approved these changes Jul 6, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jul 9, 2021

/retest

@kzys kzys removed the status/needs-update Awaiting contributor update label Jul 9, 2021
kzys
kzys approved these changes Jul 9, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@containerd/committers Can someone take a look and merge?

@dmcgowan dmcgowan merged commit 31d7844 into containerd:main Jul 9, 2021
@wllenyj wllenyj deleted the plugin_test branch July 12, 2021 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estesp estesp estesp left review comments

@kzys kzys kzys approved these changes

@dmcgowan dmcgowan dmcgowan approved these changes

@ktock ktock ktock approved these changes

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@wllenyj @k8s-ci-robot @mxpv @dmcgowan @fuweid @kzys @estesp @ktock