Skip to content

add more test cases for default runtime validation#512

Merged
liangchenye merged 1 commit intoopencontainers:masterfrom
Mashimiao:cases-for-default-runtime
Nov 16, 2017
Merged

add more test cases for default runtime validation#512
liangchenye merged 1 commit intoopencontainers:masterfrom
Mashimiao:cases-for-default-runtime

Conversation

@Mashimiao
Copy link
Copy Markdown

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

@Mashimiao Mashimiao added this to the v0.4.0 milestone Nov 15, 2017

// Test whether mounts are correctlt mounted
func TestValidateMounts(t *testing.T) {
// TODO mounts generation options have not been implemented
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can add this test when 'mounts generate' will be done.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added such TODO comment

// Test whether rootfs Readonly can be applied or not
func TestValidateRootFS(t *testing.T) {
g := getDefaultGenerator()
g.SetRootReadonly(true)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we can add another test like:

g.SetRootReadonly(false)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added, PTAL

@Mashimiao Mashimiao force-pushed the cases-for-default-runtime branch from f78072c to bad62c8 Compare November 15, 2017 10:06
// Test whether rootfs Readonly can be applied as true
func TestValidateRootFS(t *testing.T) {
g := getDefaultGenerator()
g.SetRootReadonly(true)
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 think we need to skip this on Windows, where the spec forbids setting readonly to true. Also, you probably need to use more-specific function names:

$ git grep '^func TestValidateRootFS' origin/pr/512
origin/pr/512:validation/validation_test.go:func TestValidateRootFS(t *testing.T) {
origin/pr/512:validation/validation_test.go:func TestValidateRootFS(t *testing.T) {

Comment thread validation/validation_test.go Outdated
assert.Nil(t, runtimeInsideValidate(g))
}

// Test whether mounts are correctlt mounted
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.

“correctlt” → “correctly”. Or just use // Validate mounts, or leave the comments off? TestValidateMounts sounds pretty clear to me even without a comment.

@Mashimiao Mashimiao force-pushed the cases-for-default-runtime branch from bad62c8 to 5ca723e Compare November 16, 2017 01:15
@Mashimiao
Copy link
Copy Markdown
Author

updated. @wking @liangchenye @q384566678 PTAL

@Mashimiao Mashimiao force-pushed the cases-for-default-runtime branch from 5ca723e to 16bc903 Compare November 16, 2017 02:10
@liangchenye
Copy link
Copy Markdown
Member

liangchenye commented Nov 16, 2017

LGTM

Approved with PullApprove

Comment thread cmd/runtimetest/main.go Outdated
}
} else {
err := testWriteAccess("/")
if err == nil {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should use err != nil.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

good catch, updated

@Mashimiao Mashimiao force-pushed the cases-for-default-runtime branch from 16bc903 to 5557d36 Compare November 16, 2017 02:38
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@zhouhao3
Copy link
Copy Markdown

zhouhao3 commented Nov 16, 2017

LGTM

Approved with PullApprove

1 similar comment
@liangchenye
Copy link
Copy Markdown
Member

liangchenye commented Nov 16, 2017

LGTM

Approved with PullApprove

@liangchenye liangchenye merged commit b78c113 into opencontainers:master Nov 16, 2017
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.

4 participants