*: Transition from tap Diagnostic(...) to YAML(...) by wking · Pull Request #533 · opencontainers/runtime-tools

wking · 2017-12-03T23:46:09Z

See ad47e7d (#439) about how node-tap handles YAML blocks vs. diagnostics.

Mashimiao · 2017-12-04T05:26:25Z

Please use gofmt -s -w validation/create.go to fix CI failure

wking · 2017-12-04T05:43:29Z

On Sun, Dec 03, 2017 at 09:26:25PM -0800, Ma Shimiao wrote: Please use `gofmt -s -w validation/create.go` to fix CI failure

Oops, thanks. Done with 206845a → e666abd.

Mashimiao · 2017-12-04T05:57:07Z

The result doesn't looks the same as result in README.md

# make RUNTIME=runc localvalidation
RUNTIME=runc tap validation/root_readonly_true.t validation/process_rlimits.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/linux_devices.t validation/linux_id_mappings.t validation/process_capabilities.t validation/process_oom_score_adj.t validation/create.t validation/hostname.t validation/linux_rootfs_propagation_shared.t validation/default.t
total ................................................... 0/0

ok

Forgot to modify Makefile?

wking · 2017-12-04T06:19:35Z

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.

Mashimiao · 2017-12-04T06:37:46Z

So strange, The following is my environment:

# which tap
/usr/bin/tap
# rpm -qf /usr/bin/tap
nodejs-tap-0.4.4-9.fc24.noarch
# file /usr/bin/tap
/usr/bin/tap: symbolic link to ../lib/node_modules/tap/bin/tap.js
# uname -r
4.13.13-200.fc26.x86_64

Mashimiao · 2017-12-04T06:54:08Z

And

# ./validation/create.t 
TAP version 13
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"
  }
  ...
ok 2 - create MUST create a new container
  ---
  {
    "reference": "https://github.com/opencontainers/runtime-spec/blob/v1.0.0/runtime.md#create"
  }
  ...
ok 3 -

I think that's not what we expect

wking · 2017-12-04T16:58:29Z

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)

wking · 2017-12-04T17:08:26Z

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 -

We expect a passing test here, but having a test description would help :p. I've added one with e666abd → a55d234, which also fixes the fourth create.t test so it hits the runtime. But I don't think either of those would be the source of your 0/0 results.

wking · 2017-12-04T18:14:01Z

Ah, possible cause for your 0/0 is that you didn't run:

$ make runtimetest validation-executables

first. I've added a guard for that to the Makefile with a55d234 → 07a3809.

Mashimiao · 2017-12-06T03:57:01Z

No, I did run $ make runtimetest validation-executables.

wking · 2017-12-06T04:01:27Z

Can you strace -f ... tap validation/mounts.t to see if tap is actually executing mounts.t?

wking · 2017-12-13T17:12:13Z

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 .YAML(…) method too.

zhouhao3 · 2017-12-20T07:49:19Z

@Mashimiao @mrunalp @liangchenye What do you think of this pr?

zhouhao3 · 2017-12-25T08:54:31Z

README.md


 ```console
 $ make runtimetest validation-executables
+$ sudo make RUNTIME=runc localvalidation


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.

Do we need to add the following?

for EXECUTABLE …

I've pushed 07a3809 → 23f547a to remove that noise from the localvalidation output.

zhouhao3 · 2017-12-27T01:32:02Z

LGTM

zhouhao3 · 2018-01-02T03:18:42Z

ping @liangchenye @Mashimiao

zhouhao3 · 2018-01-08T01:26:58Z

This is an important pr that progressed to the 0.4 version, WDYT @liangchenye .

liangchenye · 2018-01-10T08:20:35Z

It looks good to me and sorry @wking I merged 547 just now, so we have a conflict to solve.

Since the runtime-CLI interface is added now. #321
we need to change the 'create' function a lot. But we can do that in our 0.5 milestone.

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>

wking · 2018-01-10T17:03:50Z

Rebased onto master with 23f547a → e85081a.

zhouhao3 · 2018-01-11T01:24:49Z

LGTM

liangchenye · 2018-01-11T01:42:33Z

LGTM

wking force-pushed the tap-yaml branch from 796f393 to 206845a Compare December 3, 2017 23:47

wking force-pushed the tap-yaml branch from 206845a to e666abd Compare December 4, 2017 05:42

Mashimiao added this to the v0.4.0 milestone Dec 14, 2017

zhouhao3 reviewed Dec 25, 2017

View reviewed changes

wking force-pushed the tap-yaml branch from 07a3809 to 23f547a Compare December 26, 2017 22:51

wking added 5 commits January 10, 2018 09:02

validation/create: Label the state ID comparison test

a12de42

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>

wking force-pushed the tap-yaml branch from 23f547a to e85081a Compare January 10, 2018 17:03

liangchenye merged commit e6a60ee into opencontainers:master Jan 11, 2018

wking deleted the tap-yaml branch January 11, 2018 05:21

Conversation

wking commented Dec 3, 2017

Uh oh!

Mashimiao commented Dec 4, 2017

Uh oh!

wking commented Dec 4, 2017 via email

Uh oh!

Mashimiao commented Dec 4, 2017

Uh oh!

wking commented Dec 4, 2017 via email

Uh oh!

Mashimiao commented Dec 4, 2017

Uh oh!

Mashimiao commented Dec 4, 2017

Uh oh!

wking commented Dec 4, 2017 via email

Uh oh!

wking commented Dec 4, 2017 via email

Uh oh!

wking commented Dec 4, 2017

Uh oh!

Mashimiao commented Dec 6, 2017

Uh oh!

wking commented Dec 6, 2017

Uh oh!

wking commented Dec 13, 2017

Uh oh!

zhouhao3 commented Dec 20, 2017

Uh oh!

zhouhao3 Dec 25, 2017

Choose a reason for hiding this comment

Uh oh!

wking Dec 26, 2017

Choose a reason for hiding this comment

Uh oh!

zhouhao3 commented Dec 27, 2017 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouhao3 commented Jan 2, 2018

Uh oh!

zhouhao3 commented Jan 8, 2018

Uh oh!

liangchenye commented Jan 10, 2018

Uh oh!

wking commented Jan 10, 2018

Uh oh!

zhouhao3 commented Jan 11, 2018 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liangchenye commented Jan 11, 2018 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zhouhao3 commented Dec 27, 2017 •

edited by caniszczyk

Loading

zhouhao3 commented Jan 11, 2018 •

edited by caniszczyk

Loading

liangchenye commented Jan 11, 2018 •

edited by caniszczyk

Loading