Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cmd/runtimetest/main: Use TAP diagnostics for errors #439

Merged
merged 5 commits into from
Dec 1, 2017
Merged

cmd/runtimetest/main: Use TAP diagnostics for errors #439

merged 5 commits into from
Dec 1, 2017

Conversation

wking
Copy link
Contributor

@wking wking commented Jul 28, 2017 •
edited
Loading

This lets us print SHOULD violations even if they're non-fatal. It also gets us back to consumable TAP output. I added a /foo entry to defaultFS for testing, and before this commit:

$ ./runtimetest
TAP version 13
ok 1 - root filesystem
ok 2 - hostname
ok 3 - mounts
not ok 4 - capabilities
ok 5 - default symlinks
ok 6 - default devices
ok 7 - linux devices
not ok 8 - linux process
ok 9 - masked paths
ok 10 - oom score adj
ok 11 - read only paths
not ok 12 - rlimits
ok 13 - sysctls
ok 14 - uid mappings
ok 15 - gid mappings
1..15
3 errors occurred:

* Unexpected bounding capability chown set for process
* UID expected: 1, actual: 1000
* Wrong rlimit value:
$ echo $?
1

The TAP 13 spec doesn't cover script exit-code handling, but that exit code confuses prove unless you set --ignore-exit:

$ prove ./runtimetest
./runtimetest .. 1/? 3 errors occurred:

* Unexpected bounding capability chown set for process
* UID expected: 1, actual: 1000
* Wrong rlimit value:
./runtimetest .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 4/16 subtests

Test Summary Report
-------------------
./runtimetest (Wstat: 256 Tests: 16 Failed: 4)
  Failed tests:  4, 6, 9, 13
  Non-zero exit status: 1
Files=1, Tests=16,  0 wallclock secs ( 0.02 usr +  0.00 sys =  0.02 CPU)
Result: FAIL

Note the “Dubious, test returned 1”. And with the diagnostics written to stderr, there's no way to have prove catch that.

With this commit, we're back to TAP-compatible output and exit codes, using TAP's diagnostics to share the failure details.

$ ./runtimetest
TAP version 13
ok 1 - root filesystem
ok 2 - hostname
ok 3 - mounts
not ok 4 - capabilities
# Unexpected bounding capability chown set for process
ok 5 - default symlinks
ok 6 - default file system
# /foo SHOULD exist and expected type is tmpfs
# Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config-linux.md#default-filesystems
ok 7 - default devices
ok 8 - linux devices
not ok 9 - linux process
# UID expected: 1, actual: 1000
ok 10 - masked paths
ok 11 - oom score adj
ok 12 - read only paths
not ok 13 - rlimits
# Wrong rlimit value:
ok 14 - sysctls
ok 15 - uid mappings
ok 16 - gid mappings
1..16
$ echo $?
0

You can use prove without --ignore-edit to summarize:

$ prove ./runtimetest
./runtimetest .. Failed 3/16 subtests

Test Summary Report
-------------------
./runtimetest (Wstat: 0 Tests: 16 Failed: 3)
  Failed tests:  4, 9, 13
Files=1, Tests=16,  0 wallclock secs ( 0.01 usr +  0.00 sys =  0.01 CPU)
Result: FAIL

And prove knows that the tests failed from TAP, with the script exit code saying “we were able to run the tests” and not speaking to pass/fail:

$ echo $?
1

If that's too compact, you can ask prove to show failures and comments:

$ prove -fo ./runtimetest
./runtimetest .. 1/?
not ok 4 - capabilities
# Unexpected bounding capability chown set for process
# /foo SHOULD exist and expected type is tmpfs
# Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config-linux.md#default-filesystems
not ok 9 - linux process
# UID expected: 1, actual: 1000
not ok 13 - rlimits
# Wrong rlimit value:
./runtimetest .. Failed 3/16 subtests

Test Summary Report
-------------------
./runtimetest (Wstat: 0 Tests: 16 Failed: 3)
  Failed tests:  4, 9, 13
Files=1, Tests=16,  0 wallclock secs ( 0.02 usr +  0.00 sys =  0.02 CPU)
Result: FAIL

You can also turn that SHOULD violation into a failure by cranking up the compliance level:

$ prove -fo ./runtimetest :: --compliance-level SHOULD
./runtimetest .. 1/?
not ok 4 - capabilities
# Unexpected bounding capability chown set for process
not ok 6 - default file system
# /foo SHOULD exist and expected type is tmpfs
# Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config-linux.md#default-filesystems
not ok 9 - linux process
# UID expected: 1, actual: 1000
not ok 13 - rlimits
# Wrong rlimit value:
./runtimetest .. Failed 4/16 subtests

Test Summary Report
-------------------
./runtimetest (Wstat: 0 Tests: 16 Failed: 4)
  Failed tests:  4, 6, 9, 13
Files=1, Tests=16,  0 wallclock secs ( 0.02 usr +  0.00 sys =  0.02 CPU)
Result: FAIL

Fixed #362.

Related to #308 in that we're improving the TAP output and removing go-multierror, but otherwise unrelated to that PR.

@wking
Copy link
Contributor Author

wking commented Aug 9, 2017

Rebased around #437 with 521602f → 9fe8199.

@wking wking mentioned this pull request Oct 2, 2017
Closed
wking added a commit to wking/ocitools-v2 that referenced this pull request Oct 3, 2017
@wking
specerror: Add FilterError helper
91aa0e9 
This doesn't save much time for folks who are iterating over the
removed errors (e.g. to print warnings about them), but Aleksa asked
for it [1].

I haven't used it in runtimetest.  I'm waiting for the in-flight
7aa3987 (cmd/runtimetest/main: Use TAP diagnostics for errors,
2017-07-28, opencontainers#439) to settle, since that commit cleans up some of the
runtimetest error-processing code with:

  validations := defaultValidations
  if platform == "linux" {
    validations = append(validations, linuxValidations...)
  }

and a single loop over the constructed validations array.

[1]: opencontainers#492 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/ocitools-v2 that referenced this pull request Oct 3, 2017
@wking
specerror: Add FilterError helper
be12cce 
This doesn't save much time for folks who are iterating over the
removed errors (e.g. to print warnings about them), but Aleksa asked
for it [1].

I haven't used it in runtimetest.  I'm waiting for the in-flight
7aa3987 (cmd/runtimetest/main: Use TAP diagnostics for errors,
2017-07-28, opencontainers#439) to settle, since that commit cleans up some of the
runtimetest error-processing code with:

  validations := defaultValidations
  if platform == "linux" {
    validations = append(validations, linuxValidations...)
  }

and a single loop over the constructed validations array.

I initially had FilterError returning (filtered, removed), but golint
didn't like that:

  error should be the last type when returning multiple items

[1]: opencontainers#492 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/ocitools-v2 that referenced this pull request Oct 4, 2017
@wking
specerror: Add FilterError helper
1cbca20 
This doesn't save much time for folks who are iterating over the
removed errors (e.g. to print warnings about them), but Aleksa asked
for it [1].

I haven't used it in runtimetest.  I'm waiting for the in-flight
7aa3987 (cmd/runtimetest/main: Use TAP diagnostics for errors,
2017-07-28, opencontainers#439) to settle, since that commit cleans up some of the
runtimetest error-processing code with:

  validations := defaultValidations
  if platform == "linux" {
    validations = append(validations, linuxValidations...)
  }

and a single loop over the constructed validations array.

I initially had FilterError returning (filtered, removed), but golint
didn't like that:

  error should be the last type when returning multiple items

[1]: opencontainers#492 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking mentioned this pull request Oct 10, 2017
Merged
wking added a commit to wking/ocitools-v2 that referenced this pull request Oct 10, 2017
@wking
specerror: Add SplitLevel helper
6089f63 
This doesn't save much time for folks who are iterating over the
removed errors (e.g. to print warnings about them), but Aleksa asked
for it [1] and Ma suggested the helper struct [2].

I haven't used it in runtimetest.  I'm waiting for the in-flight
7aa3987 (cmd/runtimetest/main: Use TAP diagnostics for errors,
2017-07-28, opencontainers#439) to settle, since that commit cleans up some of the
runtimetest error-processing code with:

  validations := defaultValidations
  if platform == "linux" {
    validations = append(validations, linuxValidations...)
  }

and a single loop over the constructed validations array.

[1]: opencontainers#492 (comment)
[2]: opencontainers#492 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@Mashimiao Mashimiao added this to the v0.4.0 milestone Nov 1, 2017
@wking wking mentioned this pull request Nov 15, 2017
Merged
@Mashimiao
Copy link

@wking can you rebase this PR? If the PR is not ready to merge, it's a little hard for us to review.

@wking
Copy link
Contributor Author

wking commented Nov 22, 2017

@wking can you rebase this PR?

I'll try to rebase tomorrow, but the PR topic message should have sufficient detail to review the user impact. If you feel it doesn't, what more information would you like?

@Mashimiao
Copy link

If your PR is not ready for merging, I think most reviewers will not have interest to review it.
Keep PR ready for reviewing, I think that's the basic we need to do.

@wking
cmd/runtimetest/main: Use TAP diagnostics for errors
9422eec 
This lets us print SHOULD violations even if they're non-fatal.  It
also gets us back to consumable TAP output.  I added a /foo entry to
defaultFS for testing, and before this commit:

  $ ./runtimetest
  TAP version 13
  not ok 1 - root filesystem
  ok 2 - hostname
  not ok 3 - process
  not ok 4 - mounts
  not ok 5 - user
  ok 6 - rlimits
  ok 7 - capabilities
  ok 8 - default symlinks
  ok 9 - default file system
  ok 10 - default devices
  ok 11 - linux devices
  not ok 12 - linux process
  ok 13 - masked paths
  ok 14 - oom score adj
  ok 15 - read only paths
  ok 16 - rootfs propagation
  ok 17 - sysctls
  ok 18 - uid mappings
  ok 19 - gid mappings
  1..19
  6 errors occurred:

  * rootfs must not be readonly
  Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#root
  * Cwd expected: /, actual: /home/wking/.local/lib/go/src/github.com/opencontainers/runtime-tools
  * mounts[1] {/tmp tmpfs none []} does not exist
  Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#mounts
  * mounts[2] {/dev devtmpfs devtmpfs []} mounted before mounts[0] {/tmp tmpfs none []}
  Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#mounts
  * UID expected: 1, actual: 1000
  * Process arguments expected: ./runtimetest, actual: sh
  $ echo $?
  1

The TAP 13 spec doesn't cover script exit-code handling [1], but that
exit code confuses prove unless you set --ignore-exit:

  $ prove ./runtimetest
  ./runtimetest .. 1/? 6 errors occurred:

  * rootfs must not be readonly
  Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#root
  * Cwd expected: /, actual: /home/wking/.local/lib/go/src/github.com/opencontainers/runtime-tools
  * mounts[1] {/tmp tmpfs none []} does not exist
  Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#mounts
  * mounts[2] {/dev devtmpfs devtmpfs []} mounted before mounts[0] {/tmp tmpfs none []}
  Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#mounts
  * UID expected: 1, actual: 1000
  * Process arguments expected: ./runtimetest, actual: sh
  ./runtimetest .. Dubious, test returned 1 (wstat 256, 0x100)
  Failed 5/19 subtests

  Test Summary Report
  -------------------
  ./runtimetest (Wstat: 256 Tests: 19 Failed: 5)
    Failed tests:  1, 3-5, 12
    Non-zero exit status: 1
  Files=1, Tests=19,  0 wallclock secs ( 0.02 usr +  0.00 sys =  0.02 CPU)
  Result: FAIL

Note the "Dubious, test returned 1".  And with the diagnostics written
to stderr, there's no way to have prove catch that.

With this commit, we're back to TAP-compatible output and exit codes,
using TAP's diagnostics [2] to share the failure details.

  $ ./runtimetest
  TAP version 13
  not ok 1 - root filesystem
  # rootfs must not be readonly
  # Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#root
  ok 2 - hostname
  not ok 3 - process
  # Cwd expected: /, actual: /home/wking/.local/lib/go/src/github.com/opencontainers/runtime-tools
  not ok 4 - mounts
  # mounts[1] {/tmp tmpfs none []} does not exist
  # Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#mounts
  not ok 5 - mounts
  # mounts[2] {/dev devtmpfs devtmpfs []} mounted before mounts[0] {/tmp tmpfs none []}
  # Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#mounts
  not ok 6 - user
  # UID expected: 1, actual: 1000
  ok 7 - rlimits
  ok 8 - capabilities
  ok 9 - default symlinks
  ok 10 - default file system
  ok 11 - default devices
  ok 12 - linux devices
  not ok 13 - linux process
  # Process arguments expected: ./runtimetest, actual: sh
  ok 14 - masked paths
  ok 15 - oom score adj
  ok 16 - read only paths
  ok 17 - rootfs propagation
  ok 18 - sysctls
  ok 19 - uid mappings
  ok 20 - gid mappings
  1..20
  $ echo $?
  0

You can use prove without --ignore-edit to summarize:

  $ prove ./runtimetest
  ./runtimetest .. Failed 6/20 subtests

  Test Summary Report
  -------------------
  ./runtimetest (Wstat: 0 Tests: 20 Failed: 6)
    Failed tests:  1, 3-6, 13
  Files=1, Tests=20,  0 wallclock secs ( 0.03 usr +  0.00 sys =  0.03 CPU)
  Result: FAIL

And prove knows that the tests failed from TAP, with the script exit
code saying "we were able to run the tests" and not speaking to
pass/fail:

  $ echo $?
  1

If that's too compact, you can ask prove to show failures and
comments:

  $ prove -fo ./runtimetest
  ./runtimetest .. 1/?
  not ok 1 - root filesystem
  # rootfs must not be readonly
  # Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#root
  not ok 3 - process
  # Cwd expected: /, actual: /home/wking/.local/lib/go/src/github.com/opencontainers/runtime-tools
  not ok 4 - mounts
  # mounts[1] {/tmp tmpfs none []} does not exist
  # Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#mounts
  not ok 5 - mounts
  # mounts[2] {/dev devtmpfs devtmpfs []} mounted before mounts[0] {/tmp tmpfs none []}
  # Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#mounts
  not ok 6 - user
  # UID expected: 1, actual: 1000
  not ok 13 - linux process
  # Process arguments expected: ./runtimetest, actual: sh
  ./runtimetest .. Failed 6/20 subtests

  Test Summary Report
  -------------------
  ./runtimetest (Wstat: 0 Tests: 20 Failed: 6)
    Failed tests:  1, 3-6, 13
  Files=1, Tests=20,  0 wallclock secs ( 0.03 usr +  0.00 sys =  0.03 CPU)
  Result: FAIL

You can also turn that SHOULD error into a failure by cranking up the
compliance level:

  $ prove -fo ./runtimetest :: --compliance-level SHOULD

although I don't have any interesting SHOULD violations between my
test config.json and my host system.

[1]: https://testanything.org/tap-version-13-specification.html#todo
[2]: https://testanything.org/tap-version-13-specification.html#diagnostics

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Nov 24, 2017 via email

@Mashimiao
Copy link

@wking I'm sorry about that if review process makes you unhappy.
I think there may have a little problem on PR review process. We should consider old PRs first if there are not other PRs in higher priority.
We will improve our review process in the future.
And thanks for your comments and contributions.

@wking
Copy link
Contributor Author

wking commented Nov 24, 2017 via email

@Mashimiao
Copy link

Mashimiao commented Nov 24, 2017 via email

@wking wking mentioned this pull request Nov 24, 2017
Merged
@Mashimiao
Copy link

Mashimiao commented Nov 24, 2017 •
edited
Loading

I'm OK with using t.Diagnostic(). But I think there is a problem, there is not prove in busybox and currently we just run runtimetest.
The result is no matter what errors occur, TAP always says test PASS, as following

TAP version 13
ok 1 - root filesystem
ok 2 - hostname
ok 3 - process
ok 4 - mounts
ok 5 - user
ok 6 - rlimits
ok 7 - capabilities
ok 8 - default symlinks
ok 9 - default file system
ok 10 - default devices
ok 11 - linux devices
ok 12 - linux process
not ok 13 - masked paths
# /masktest should not be readable
ok 14 - oom score adj
ok 15 - read only paths
ok 16 - rootfs propagation
ok 17 - sysctls
ok 18 - uid mappings
ok 19 - gid mappings
1..19
--- PASS: TestValidateMaskedPaths (0.18s)

I think that's not so suitable, because the result is opposite with fact.

@wking
Copy link
Contributor Author

wking commented Nov 24, 2017

But I think there is a problem, there is not prove in busybox and currently we just run runtimetest.

I see two possible solutions:

a. Drop my change to the exit code, and expect folks who do use prove to set --ignore-exit.
b. Use prove (or another TAP consumer) outside of the container.

I'm fine with (a) if folks want a short-term fix, but I think (b) is a better long-term approach (e.g. it allows easily tunable verbosity for #308). I'm fine working up either, or other alternatives, as part of this PR.

@Mashimiao
Copy link

considering #308, I prefer b. Ping @opencontainers/runtime-tools-maintainers

@wking
generate: Move generate_test.go from validation
1c2dca0 
The generate package already imports the validate package, so this
isn't creating an import cycle.  And as a generation smoke test, this
doesn't belong in validation (which is about compliance-testing
runtimes).

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Nov 26, 2017 via email

@liangchenye
Copy link
Member

+1 for the b plan. It is a big improvement.
We also need to add a document to help user to get the prove tool, or add a script to contrib directory.

@liangchenye
Copy link
Member

@Mashimiao @q384566678 I merged 3 minor fixes just now and I think we would better stop working on other PRs until we merging this one and #308.

@Mashimiao
Copy link

I also want this PR landed as soon as possible.
But considering we have been behind after schedule a lot, I think we can work on generate related PRs while waiting #439 and #308.
Of course, our merging based on those PRs will not affect the two PRs.
How do you think about it? @liangchenye

@wking
Copy link
Contributor Author

wking commented Nov 28, 2017

We also need to add a document to help user to get the prove tool…

Pushed with b2677cb → b2c08bf.

@liangchenye liangchenye mentioned this pull request Nov 29, 2017
Open
@Mashimiao
Copy link

@wking I think @liangchenye has found the root cause, would you like to update the PR?

@wking
Copy link
Contributor Author

wking commented Nov 30, 2017

... would you like to update the PR?

I've been working on it, but don't have everything fixed yet. Maybe by tonight or tomorrow.

@Mashimiao
Copy link

Got it, thanks.

@wking
Copy link
Contributor Author

wking commented Nov 30, 2017 via email

@Mashimiao
Copy link 

validation/linux_gid_mappings.t ................... Failed 1/19 subtests 
validation/linux_rootfs_propagation_unbindable.t .. failed to create the container
rootfsPropagation=unbindable is not supported
exit status 1
validation/linux_rootfs_propagation_unbindable.t .. Dubious, test returned 1 (wstat 256, 0x100)
No subtests run 
validation/linux_readonly_paths.t ................. ok    
validation/linux_masked_paths.t ................... Failed 1/19 subtests

It seems except container created failed, other the failed reasons are not output?

@wking
Copy link
Contributor Author

wking commented Nov 30, 2017 via email

@wking wking force-pushed the tap-diagnostics branch 2 times, most recently from d2f506d to 0a919c0 Compare November 30, 2017 17:09
@wking
validation: Use prove(1) as a TAP harness
e11b77f 
Capture stdout and stderr from create invocations, because we don't
want to pollute the TAP output with things like runc's:

  Incorrect Usage.
  ...

(which is for some reason printed to stdout) or:

  runc: "create" requires exactly 1 argument(s)

which is printed to stderr.  Instead, show the captured stderr as a
diagnostic, and hide the stdout completely.  Unless stderr is empty,
in which case show stdout in case it contains something useful.

Most of these tests are broken because we aren't collecting the
container exit code or post-start stdout.  But the tests haven't been
doing that since the create/start split in 15577bd (add runtime
struct; add create test, 2017-08-24, opencontainers#447) anyway [1].  This commit
just makes that more obvious.

The patsubst and wildcard Makefile syntax is documented in [2].  The
$(VALIDATION_TESTS) rule uses the static pattern rule syntax [3].

And use 'console' instead of 'sh' in the README, because these are
shell sessions, not scripts.  See [4,5].  I don't have a working runc
locally, so the mock results based on a dummy runtime script.

[1]: opencontainers#447 (comment)
[2]: https://www.gnu.org/software/make/manual/html_node/Wildcard-Function.html
[3]: https://www.gnu.org/software/make/manual/html_node/Static-Usage.html#Static-Usage
[4]: https://help.github.com/articles/creating-and-highlighting-code-blocks/#syntax-highlighting
[5]: https://github.com/github/linguist/blob/v5.3.3/lib/linguist/languages.yml#L4249-L4260

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
validation/util/container: Remove bundle even if delete fails
0a919c0 
When the container failed in creation, delete will fail, but we still
want to remove the test bundle directory.

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Makefile: Change from prove to node-tap
ad47e7d 
For better reporting, including skips.  Prove (at least as of
TAP::Harness v3.35_01 and Perl v5.22.3) seems to report skips only
when there were also failures.  For example, here's a skip-only test:

  $ validation/mounts.t
  TAP version 13
  1..1
  ok 1 # SKIP TODO: mounts generation options have not been implemented
  $ prove validation/mounts.t
  validation/mounts.t .. ok
  All tests successful.
  Files=1, Tests=1,  0 wallclock secs ( 0.01 usr +  0.00 sys =  0.01 CPU)
  Result: PASS

node-tap (as of version 11.0.0) reports that skip:

  $ tap validation/mounts.t
  validation/mounts.t ................................... 0/1
    Skipped: 1
       TODO: mounts generation options have not been implemented

  total ................................................. 0/1

    0 passing (27.297ms)
    1 pending

And here's a skip with a failure test:

  $ ./test-skip
  TAP version 13
  1..2
  not ok 1 - failing
  ok 2 # SKIP: skipping

Prove warns about the skip now:

  $ prove test-skip
  test-skip .. Failed 1/2 subtests
          (less 1 skipped subtest: 0 okay)

  Test Summary Report
  -------------------
  test-skip (Wstat: 0 Tests: 2 Failed: 1)
    Failed test:  1
  Files=1, Tests=2,  0 wallclock secs ( 0.02 usr +  0.00 sys =  0.02 CPU)
  Result: FAIL

But node-tap has a nicer warning:

  $ tap ./test-skip
  ./test-skip ........................................... 0/2
    not ok failing

    Skipped: 1
       : skipping

  total ................................................. 0/2

    0 passing (39.088ms)
    1 pending
    1 failing

Similarly, node-tap does a better job handling YAML blocks [1]:

  $ ./test-yaml
  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
    ...

Prove either shows no diagnostics:

  $ prove test-yaml
  test-yaml .. Failed 2/4 subtests

  Test Summary Report
  -------------------
  test-yaml (Wstat: 0 Tests: 4 Failed: 2)
    Failed tests:  2, 4
  Files=1, Tests=4,  0 wallclock secs ( 0.02 usr +  0.00 sys =  0.02 CPU)
  Result: FAIL

or it shows all the diagnostics (even for successful tests):

  $ prove --comments test-yaml
  test-yaml .. 1/?
  # success
  # failure
  test-yaml .. Failed 2/4 subtests

  Test Summary Report
  -------------------
  test-yaml (Wstat: 0 Tests: 4 Failed: 2)
    Failed tests:  2, 4
  Files=1, Tests=4,  0 wallclock secs ( 0.02 usr +  0.00 sys =  0.02 CPU)
  Result: FAIL

I can't find a way to get Prove to show the YAML blocks.  node-tap, on
the other hand, does the right thing with YAML blocks:

  $ tap ./test-yaml
  ./test-yaml ........................................... 2/4
    not ok failure diagnostic
    not ok failure YAML
      message: failure

  total ................................................. 2/4

    2 passing (42.409ms)
    2 failing

We don't use YAML blocks at the moment, but I'm going to transition to
them later.

[1]: http://testanything.org/tap-version-13-specification.html#yaml-blocks

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Nov 30, 2017

YAML-block support PR filed upstream in mndrix/tap-go#7. If/when that lands, I'll transition from Diagnostic(f) to YAML here.

@wking wking mentioned this pull request Nov 30, 2017
Merged
@Mashimiao
Copy link

Mashimiao commented Dec 1, 2017 •
edited by caniszczyk
Loading

LGTM

Approved with PullApprove

1 similar comment
@liangchenye
Copy link
Member

liangchenye commented Dec 1, 2017 •
edited by caniszczyk
Loading

LGTM

Approved with PullApprove

@liangchenye liangchenye merged commit 36cf932 into opencontainers:master Dec 1, 2017
wking added a commit to wking/ocitools-v2 that referenced this pull request Dec 3, 2017
@wking
vendor/github.com/mndrix/tap-go: Bump to 629fa40
53403c3 
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.

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/ocitools-v2 that referenced this pull request Dec 3, 2017
@wking
*: Transition from tap Diagnostic(...) to YAML(...)
796f393 
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>
@wking wking mentioned this pull request Dec 3, 2017
Merged
wking added a commit to wking/ocitools-v2 that referenced this pull request Dec 3, 2017
@wking
vendor/github.com/mndrix/tap-go: Bump to 629fa40
aeaf786 
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>
wking added a commit to wking/ocitools-v2 that referenced this pull request Dec 3, 2017
@wking
*: Transition from tap Diagnostic(...) to YAML(...)
206845a 
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>
wking added a commit to wking/ocitools-v2 that referenced this pull request Dec 4, 2017
@wking
*: Transition from tap Diagnostic(...) to YAML(...)
e666abd 
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>
wking added a commit to wking/ocitools-v2 that referenced this pull request Dec 4, 2017
@wking
Makefile: Clearer warning on missing validation executable(s)
07a3809 
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 wking deleted the tap-diagnostics branch December 13, 2017 17:05
wking added a commit to wking/ocitools-v2 that referenced this pull request Dec 26, 2017
@wking
Makefile: Clearer warning on missing validation executable(s)
23f547a 
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 added a commit to wking/ocitools-v2 that referenced this pull request Jan 10, 2018
@wking
vendor/github.com/mndrix/tap-go: Bump to 629fa40
0c66fe9 
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>
wking added a commit to wking/ocitools-v2 that referenced this pull request Jan 10, 2018
@wking
*: Transition from tap Diagnostic(...) to YAML(...)
b880d57 
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>
wking added a commit to wking/ocitools-v2 that referenced this pull request Jan 10, 2018
@wking
Makefile: Clearer warning on missing validation executable(s)
e85081a 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@wking @Mashimiao @liangchenye