-
Notifications
You must be signed in to change notification settings - Fork 142
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
*: Transition from tap Diagnostic(...) to YAML(...) #533
Conversation
Please use |
The result doesn't looks the same as result in README.md
Forgot to modify Makefile? |
On Sun, Dec 03, 2017 at 09:57:08PM -0800, Ma Shimiao wrote:
# make RUNTIME=runc localvalidation
The README uses ‘sudo’, but I'll assume your ‘#’ prompt means you're
running as root.
RUNTIME=runc tap validation/root_readonly_true.t … validation/default.t
total ................................................... 0/0
That is very strange. I'm not able to reproduce that myself. Is it
actually node-tap? Is it executing all of the listed test
executables?
Forgot to modify Makefile?
This PR should not require any Makefile updates.
|
So strange, The following is my environment:
|
And
I think that's not what we expect |
On Sun, Dec 03, 2017 at 10:37:46PM -0800, Ma Shimiao wrote:
# rpm -qf /usr/bin/tap
nodejs-tap-0.4.4-9.fc24.noarch
I haven't looked too closely, but it seems like nodejs-tap improved
it's YAML handling in 1.0.0 [1]. You may want to test with a more
recent version (I'm using the current 11.0.0). If the issue is
nodejs-tap, you can also isolate the problem by using a script that
just echos some canned TAP:
$ cat test-yaml
#!/bin/sh
cat <<EOF
TAP version 13
1..4
ok 1 - success diagnostic
# success
not ok 2 - failure diagnostic
# failure
ok 3 - success YAML
---
message: success
...
not ok 4 - failure YAML
---
message: failure
...
EOF
$ tap ./test-yaml
./test-yaml ........................................... 2/4
not ok failure diagnostic
not ok failure YAML
message: failure
total ................................................. 2/4
2 passing (40.267ms)
2 failing
[1]: tapjs/tapjs#109 (comment)
|
On Sun, Dec 03, 2017 at 10:54:09PM -0800, Ma Shimiao wrote:
ok 1 - create MUST generate an error if the ID is not provided
---
{
"error": "exit status 1",
"reference": "https://github.com/opencontainers/runtime-spec/blob/v1.0.0/runtime.md#create",
"stderr": "/usr/local/sbin/runc: \"create\" requires exactly 1 argument(s)\n"
}
...
This is what we expect. runc errored, and we expected an error, so
the test passes.
ok 3 -
|
No, I did run |
Can you |
This is a followup to #439. Can we add it to the 0.4.0 milestone and land it before #308? I'd rather switch #308 over to the |
@Mashimiao @mrunalp @liangchenye What do you think of this pr? |
@@ -39,15 +39,18 @@ $ npm install tap | |||
|
|||
```console | |||
$ make runtimetest validation-executables | |||
$ sudo make RUNTIME=runc localvalidation |
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.
Do we need to add the following?
for EXECUTABLE in runtimetest validation/linux_rootfs_propagation_shared.t validation/create.t validation/linux_gid_mappings.t validation/linux_rootfs_propagation_unbindable.t validation/linux_readonly_paths.t validation/linux_masked_paths.t validation/mounts.t validation/process.t validation/root_readonly_false.t validation/linux_sysctl.t validation/default.t validation/linux_devices.t validation/process_capabilities.t validation/process_oom_score_adj.t validation/process_rlimits.t validation/root_readonly_true.t validation/hostname.t validation/linux_uid_mappings.t; \ do \ test -x "${EXECUTABLE}"; \ done || (echo "missing test executable; run 'make runtimetest validation-executables'" >&2 && exit 1)
The other looks good to me.
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.
ping @liangchenye @Mashimiao |
This is an important pr that progressed to the 0.4 version, WDYT @liangchenye . |
To pick up t.YAML(...) for more visible error messages. See ad47e7d (Makefile: Change from prove to node-tap, 2017-11-30, opencontainers#439) about how node-tap handles YAML blocks vs. diagnostics. Generated with: $ go get -u github.com/mndrix/tap-go $ godep update github.com/mndrix/tap-go $ emacs Godeps/Godeps.json # remove gopkg.in/yaml.v2 entry $ rm -rf vendor/gopkg.in/yaml.v2 $ git add vendor/github.com/mndrix We don't need gopkg.in/yaml.v2 because we're using tap-go's JSON output. Signed-off-by: W. Trevor King <wking@tremily.us>
See ad47e7d (Makefile: Change from prove to node-tap, 2017-11-30, opencontainers#439) about how node-tap handles YAML blocks vs. diagnostics. The README's localvalidation line is back after I accidentally removed it in ad47e7d. Signed-off-by: W. Trevor King <wking@tremily.us>
So we get: ok 3 - 'state' MUST return the state of a container instead of: ok 3 - Signed-off-by: W. Trevor King <wking@tremily.us>
So validation/create.t gives: ok 4 - create MUST generate an error if the ID provided is not unique --- { "error": "exit status 1", "reference": "https://github.com/opencontainers/runtime-spec/blob/v1.0.0/runtime.md#create", "stderr": "container with id exists: 4a142dd0-e5bc-48b3-9abd-49c1a8f7c498\n" } ... instead of: ok 4 - create MUST generate an error if the ID provided is not unique --- { "error": "open /tmp/ocitest842577116/stdout-178f8311-9cc7-42c5-b91e-abbe29eaf582: file exists", "reference": "https://github.com/opencontainers/runtime-spec/blob/v1.0.0/runtime.md#create" } ... The old code hit the internal error, while the new code is checking for a runtime error. Signed-off-by: W. Trevor King <wking@tremily.us>
Instead of reporting total ................................................. 0/0 and passing (which node-tap seems to do when given missing files as arguments), report: missing test executable; run 'make runtimetest validation-executables' and error out. Having a separate step to build the executables is useful for: * Not building them as root, which reduces the change of security issues by using as little privilege as possible. * Not rebuilding them on each test, since we haven't told Make about the full dependency tree for the test executables. The separate build step was introduced in e11b77f (validation: Use prove(1) as a TAP harness, 2017-11-25, opencontainers#439), but I didn't do a good job of motivating it in that commit message. Signed-off-by: W. Trevor King <wking@tremily.us>
See ad47e7d (#439) about how node-tap handles YAML blocks vs. diagnostics.