implement start operation tests #578
Conversation
|
validation/start.go
Outdated
| uuid "github.com/satori/go.uuid" | ||
| ) | ||
|
|
||
| func wrap(t *tap.T, expected bool, specErr error, detailedErr error) { |
There was a problem hiding this comment.
This is probably useful/generic enough to be a public function in validation/util/test.go. Maybe SpecErrorOk()?
validation/start.go
Outdated
| r.Create() | ||
| // start a `created` container | ||
| err = r.Start() | ||
| outputData, _ := ioutil.ReadFile(output) |
There was a problem hiding this comment.
This is racy. start should block until the user-specified program has executed, but there's no guarantee about how far along that program is in its execution. That means this read could be happening either before or after the user-specified program gets around to writing to output. The coarse guard for this would be to sleep for some safe-enough time. The better, but maybe Linux-specific, guard would be to use inotify(7) to block until the file was updated (or some safe-enough time had passed) and compare the measured write time with the start-invocation time (the write time should be after start was invoked and also after any prestart hooks completed).
There was a problem hiding this comment.
I'll add time sleep here.
validation/start.go
Outdated
| // must generate an error | ||
| wrap(t, err != nil, specerror.NewError(specerror.StartNotCreatedGenError, fmt.Errorf("attempting to `start` a container that is not `created` MUST generate an error"), rspecs.Version), err) | ||
|
|
||
| outputData, _ = ioutil.ReadFile(output) |
There was a problem hiding this comment.
You shouldn't be ignoring the returned error here (or in your other ReadFile calls). If one of those errors, you can die with a non-zero exit. node-tap will complain about that:
$ cat validation/test
#!/bin/sh
cat <<EOF
TAP version 13
ok 1 - ok
EOF
exit 1
$ tap ./validation/test
./validation/test ..................................... 1/2
not ok ./validation/test
timeout: 30000
file: ./validation/test
command: ./validation/test
args: []
stdio:
- 0
- pipe
- 2
cwd: /home/wking/.local/lib/go/src/github.com/opencontainers/runtime-tools
failures:
- tapError: no plan
exitCode: 1
…
validation/start.go
Outdated
| // must have no effect, it will not be something like 'process called\nprocess called\n' | ||
| wrap(t, string(outputData) == "process called\n", specerror.NewError(specerror.StartNotCreatedHaveNoEffect, fmt.Errorf("attempting to `start` a container that is not `created` MUST have no effect on the container"), rspecs.Version), err) | ||
|
|
||
| r.Delete() |
There was a problem hiding this comment.
You have another ignored error here.
validation/start.go
Outdated
| g.Spec().Process = nil | ||
| r.SetConfig(g) | ||
| err = r.Create() | ||
| // 'create' could fail according to the spec when the process is nil |
There was a problem hiding this comment.
Why “could fail”? Is this the “create can fail whenever it wants” loophole (opencontainers/runtime-spec#813, opencontainers/runtime-spec#927)? I don't see a need to call that out in this particular case. If we want, we could adjust our Create wrapper to handle this in all cases.
There was a problem hiding this comment.
If the process is nil, the config.json is invalid. It is up to the runtime to validate it in the 'create' since: The runtime MAY validate config.json against this spec.
If a runtime validates config.json in 'create', it might return error. In this case, the container is not created, we can never test 'StartWithProcUnsetGenError'. That is why I use 'Skip' in this case.
There was a problem hiding this comment.
If the process is nil, the
config.jsonis invalid.
That's not true since opencontainers/runtime-spec#701, which was part of runtime-spec v1.0.0-rc6 and later.
liangchenye
force-pushed
the
master
branch
3 times, most recently
from
7934064 to
3ba4371
Compare
|
updated
The 'StartWithProcUnsetGenError' is not changed, I replied in @wking 's comment. |
Signed-off-by: Liang Chenye <[email protected]>
|
updated PTAL @q384566678 @wking |
|
LGTM |
Signed-off-by: Liang Chenye [email protected]