add validate_test.go to do unit test by liangchenye · Pull Request #188 · opencontainers/runtime-tools

liangchenye · 2016-08-09T11:17:39Z

Only add a test case on checkSemVer in this PR.
Signed-off-by: liang chenye liangchenye@huawei.com

wking · 2016-08-09T16:38:46Z

On Tue, Aug 09, 2016 at 04:17:40AM -0700, 梁辰晔 (Liang Chenye) wrote:

-- File Changes --
A cmd/ocitools/validate_test.go (27)

You probably want to add a Makefile and/or .travis.yml entry to run
these tests from Travis.

wking · 2016-08-09T16:45:19Z

+	for _, c := range cases {
+ spec.Version = c.v
+ msg := checkSemVer(spec, "", false)
+ if len(msg) != c.expected {


Is “number of error messages” really what we want to be checking here? I'd rather have a bunch of potential version strings with true/false for whether we thought they were valid, and error if we thought a version was valid and checkSemVer gave us error messages at all (logging the error messages with t.Fatalf) or if we thought a version was invalid and checkSemVer did not give us error messages.

You are right, it is not necessary to check the number of error messages.

wking · 2016-08-09T16:57:37Z

On Tue, Aug 09, 2016 at 04:17:40AM -0700, 梁辰晔 (Liang Chenye) wrote:

A cmd/ocitools/validate_test.go (27)

While I'm in favor of Go tests for our implementation, I'm not sure
it's worth testing cmd/… helpers. I'd rather see the validation code
pulled out into a Go library (like generate/…) and test that. Then
the cmd/… stuff should be a thin wrapper around that API, the API is
tested with ‘go test’ like you have here, and the command-line wrapper
is tested with sharness like I've floated in #180.

We'll also want a way to distinguish between “that's not valid” errors
[1](which are like HTTP 400s) and “that may be valid, but we can't
check it yet” errors [2](which are like HTTP 500s). My preferred
approach for that is to use Python's unittest and skip which lets you
handle generic JSON easily (vs. the current Go based on the
runtime-spec types or a hypothetical Go framework based on
interface{}). More on that in #98.

liangchenye · 2016-09-01T11:21:35Z

I think it is a good idea to move 'validate' to a new directory like 'generate', I'll make a patch to move that.

(just back from a long time leaving)

liangchenye · 2016-09-01T13:35:20Z

commit 82d0b38 moves validate directory out of cmd directory and add a 'validator' struct just like 'generator'.

commit 8b7ba22 adds a new unit test style based on Wking's comment

@wking @mrunalp PTAL

wking · 2016-09-01T17:07:00Z

+ expected bool
+	}{
+ {rspec.Version, true},
+ {"0.0.1", false},


0.0.1 looks like valid SemVer to me. What's the error message?

Currently only support rspec.Version.
if version != rspec.Version { msgs = append(msgs, fmt.Sprintf("internal error: validate currently only handles version %s, but the supplied configuration targets %s", rspec.Version, version)) }

On Mon, Sep 05, 2016 at 03:38:57AM -0700, David Liang wrote:
{"0.0.1", false},
Currently only support rspec.Version.
So maybe leave a comment on this line that we're confirming a
shortcoming of the current test suite with this entry.

Thanks, I added a 'FIXME' to mention this.

wking · 2016-09-08T03:46:54Z

+type Validator struct {
+	spec rspec.Spec
+	rootfs string
+	hostSpecific bool


I'd rather make these properties public, and Generator.HostSpecificis public. Do you have any reason to keep any of them private?

Do we need to see and modify these? I think the 'Check*' functions are enough.

On Wed, Sep 07, 2016 at 09:04:44PM -0700, 梁辰晔 (Liang Chenye) wrote:
+type Validator struct {
spec rspec.Spec
rootfs string
hostSpecific bool
Do we need to see and modify these? I think the 'Check*' functions
are enough.
Making them public means you don't need getters/setters and you can
still support users who want to partially setup the instance when they
call NewValidator (or whatever 1) and finish the setup later. Or if
they want to peer inside to get more details on why their validation
is failing. Or who knows? I'd rather err on the side of “we're all
consenting adults” 2, because I don't see a way to make validation
go terribly wrong if a user gets or sets one of these values whenever
they like.

A downside to making things public is that changing them breaks your public API. But this is going to be a stable, boring implementation based on a stable, boring spec right ;)? I don't expect lots of property churn as we figure out what we need to validate configurations, so I'm not worried about the slight increase in API surface area dragging us down with backwards-compat issues.

liangchenye · 2016-09-08T06:46:22Z

Hi @wking , I modified codes according to your suggestion.
Just one difference: using RootfsPath other than BundlePath in Validator{} .

wking · 2016-09-08T06:46:35Z

+ return Validator{}, fmt.Errorf("Cannot find the root path %q", rootfsPath)
+	} else if !fi.IsDir() {
+ return Validator{}, fmt.Errorf("The root path %q is not a directory.", rootfsPath)
+	}


This “does the rootfs exist” check should be a separate check in Validator.CheckAll()

done in new commit!

liangchenye · 2016-09-18T12:24:16Z

Rebase by using 'runtime-tools'.
Merge two commits (one for restruct and one for unit test case) into one. The original change log can not reflect the real meaning, code changes a lot after discussions with @wking.

@mrunalp PTAL

wking · 2016-09-19T03:58:26Z

+	return Validator{spec: spec, bundlePath: bundlePath, HostSpecific: hostSpecific}
+}
+
+func NewValidatorFromPath(bundlePath string, hostSpecific bool) (Validator, error) {


I think these NewValidator* functions should return (*Validator, error).

wking · 2016-09-19T04:13:05Z

a8c02cf looks good enough to me (I think we've agreed to punt on everything I'd still like to change).

liangchenye · 2016-10-06T08:57:32Z

Travis fails because of:
#230

liangchenye · 2016-10-06T09:01:17Z

#7cd3aa1 rebased on top of current 'master'.
Only difference with '#a8c02cf ' is '7cd3aa1' includes Mashimiao's new patch:
'validate: add hard and soft limit check for rlimit'
e129a67

@Mashimiao @mrunalp PTAL

wking reviewed Aug 9, 2016
View reviewed changes

liangchenye force-pushed the gotest branch from 89251f1 to 82d0b38Compare September 1, 2016 12:29

wking reviewed Sep 1, 2016
View reviewed changes

liangchenye force-pushed the gotest branch from 8b7ba22 to 79cc4baCompare September 8, 2016 03:35

wking reviewed Sep 8, 2016
View reviewed changes

liangchenye force-pushed the gotest branch from 79cc4ba to 687c61cCompare September 8, 2016 06:41

wking reviewed Sep 8, 2016
View reviewed changes

liangchenye force-pushed the gotest branch from 687c61c to 06163feCompare September 8, 2016 14:04

liangchenye force-pushed the gotest branch 2 times, most recently from 8d5cb3a to a8c02cfCompare September 18, 2016 12:17

wking reviewed Sep 19, 2016
View reviewed changes

wking mentioned this pull request Sep 22, 2016
Drop image tools cmd/ and image/ opencontainers/image-spec#337
Merged

Mashimiao mentioned this pull request Sep 28, 2016
validate: optimize capabilites check #220
Merged

liangchenye force-pushed the gotest branch from a8c02cf to 7cd3aa1Compare October 6, 2016 08:16

Uh oh!

Conversation

liangchenye commented Aug 9, 2016

Uh oh!

wking commented Aug 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Aug 9, 2016

Uh oh!

liangchenye commented Sep 1, 2016

Uh oh!

liangchenye commented Sep 1, 2016

Uh oh!

wkingSep 1, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wkingSep 8, 2016 via email

Choose a reason for hiding this comment

Uh oh!

liangchenye commented Sep 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liangchenye commented Sep 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking commented Sep 19, 2016 via email

Uh oh!

liangchenye commented Oct 6, 2016

Uh oh!

liangchenye commented Oct 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wkingSep 1, 2016•
edited
Loading