dependency: integrate github.com/mndrix/tap-go as internal package #767
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Using methods allows us to track some state while running tests. This means we don't have to manually track test numbers. It also means we're structurally similar to Go's testing package so that we can emulate it. That should make it easier to port a test from testing to tap.
They don't start at 0 which is the zero value for nextTestNumber
Run a small TAP test using prove to make sure the output is parsed correctly.
Perform the quick check and generate TAP output as appropriate.
More on this use case in [1]. [1]: http://testanything.org/tap-version-13-specification.html#unknown-amount-and-failures
And adjust the Makefile to make it easy to build future test/*/main.go using a static pattern rule [1]. The TESTS entries are phony to force rebuilds every time, because 'go build' includes logic to avoid unnecessary rebuilds. [1]: https://www.gnu.org/software/make/manual/html_node/Static-Usage.html
tap: Teach Header to skip the plan when testCount < 1
It's very difficult to use "go get" to fetch a repository that ends with .go To avoid this problem, rename this repository to end with -go Update the canonical import path accordingly.
This commit makes all test binaries follow the naming pattern test/foo/test This allows us to reference all test binaries with a single glob pattern: test/*/test The utility of this arrangements is already evident in .gitignore which now needs only a single line and automatically ignores all future test binaries.
Instead of running several separate prove commands (one per test), we now run a single prove command which executes all tests itself. This gives us a single test summary after all tests have concluded which should scale better as we add more tests.
The test programs in test/*/main.go all import "github.com/mndrix/tap-go". By default Go will look in the user's GOPATH to find that package. That means a user working on tap-go itself has to install the untested, development version of the package into his GOPATH before he can run the test suite. That's inconvenient and likely to break other code which relies on a working version of tap-go. The hack in this commit creates a private GOPATH within our repository which only includes the package under development. Running the test suite always uses the latest code in the repository and doesn't require our developers to manually adjust their own GOPATH each time.
The old pattern said "this rule depends on whatever files exist and match this glob". The new pattern says "this rules depends on test/auto/test and test/check/test and ... regardless of whether they exist yet". Without this change "make all" only ran tests which had already been compiled.
This is a step towards sending TAP output to an arbitrary io.Writer
In some contexts a TAP test can't send output directly to stdout. Instead it should be sent to some other writer (HTTP stream, buffer, etc). Allow this usage without breaking existing users.
The previous test suite only included passing tests. We want to cover failing tests too.
These are sometimes helpful in complex tests.
These produce diagnostic output, as defined by the TAP spec.
Use two harnesses, where the inner harness has failing tests and
writes to a buffer. The outer harness checks that buffer to ensure
the output is appropriate. This lets the whole test suite pass:
$ make
...
All tests successful.
Files=6, Tests=15, 0 wallclock secs ( 0.05 usr 0.01 sys + 0.00 cusr 0.01 csys = 0.07 CPU)
Result: PASS
while before this commit it failed:
$ make
...
Test Summary Report
-------------------
test/failing/test (Wstat: 0 Tests: 2 Failed: 2)
Failed tests: 1-2
Files=6, Tests=15, 0 wallclock secs ( 0.05 usr + 0.00 sys = 0.05 CPU)
Result: FAIL
Makefile:7: recipe for target 'all' failed
make: *** [all] Error 1
To verify the changes in opencontainers#4, I intentionally broke Diagnostic() by prefixing lines with "!" instead of "#" but the tests still passed. The spec[1] allows a test harness to silently ignore lines it doesn't understand, so that behavior makes sense. Red light green light testing[2] requires that our test be capable of failing if something is broken. This commit adds that by comparing the actual output against our expected output. Hat tip to @wking for suggesting to compare output[3]. 1: http://testanything.org/tap-version-13-specification.html#anything-else 2: http://stackoverflow.com/a/404860/174463 3: mndrix/tap-go#4 (comment)
Now that we have a function to generate diagnostic output, we don't have to construct it manually. See opencontainers#4
test/failing: Wrap failing tests in another harness
The spec [1] suggests "ok 23 # SKIP ...", but if you attempt this with:
t.Pass("# SKIP foo")
you get an extra hyphen ("ok 23 - # SKIP foo") which is parsed as a
successful test (and not a skipped test).
Michael gives the following example to motivate 'count' [2]:
if ConditionIsMet() {
t.Ok(Foo(), "foo")
t.Ok(Bar(), "bar")
} else {
t.Skip(2, "condition not met")
}
The spec example of this is in [3], showing no special syntax for
skipping multiple tests (although there is a special syntax for
skipping *all* the tests).
Also alphabetize TESTS in the Makefile.
[1]: http://testanything.org/tap-version-13-specification.html#skipping-tests
[2]: mndrix/tap-go#6 (comment)
[3]: http://testanything.org/tap-version-13-specification.html#skipping-a-few
The spec [1] suggests "not ok 13 # TODO ...", but if you attempt this
with:
t.Fail("# TODO foo")
you get an extra hyphen ("not ok 13 - # TODO foo") which is parsed as
a failed test (and not a TODO test).
I'd initially written up an alternative to Ok:
T.TODO(test bool, description string)
but Michael pointed out that future work may add additional test
methods (e.g. EqOk) and wanted to support those as well with something
like [2]:
t.TODO = true
t.Ok(Foo(), "foo")
t.EqOk(Bar(), Baz(), "bar == baz")
t.TODO = false // could be done via defer too
or [2]:
t.TODO( func () {
t.Ok(Foo(), "foo")
t.EqOk(Bar(), Baz(), "bar == baz")
})
This commit adds a .TODO boolean following Michael's first proposal.
It also adds a Todo() method returning a copy of the test-state (but
setting TODO), which you can use to avoid altering the original
test-state's TODO value. This can be useful when you don't want to
bother with a defer, or your TODO tests all live in an existing branch
of a function, or you want to chain methods for a one-off TODO test:
t.TODO().Ok(Foo(), "foo")
To keep the count synchronized between sibling test states,
nextTestNumber is now a pointer. We don't do anything to keep Writer
synchronized, because it's already part of the public API as a
non-pointer, and Michael is ok leaving this up to the caller [3].
[1]: http://testanything.org/tap-version-13-specification.html#todo-tests
[2]: mndrix/tap-go#6 (comment)
[3]: mndrix/tap-go#6 (comment)
Docs in [1]. I've used Go's JSON serializer to avoid external
dependencies, since JSON is a subset of YAML [2]. Prove is currently
choking on the results:
$ prove test/yaml/test
test/yaml/test .. Failed 1/2 subtests
Test Summary Report
-------------------
test/yaml/test (Wstat: (none) Tests: 1 Failed: 0)
Parse errors: Unsupported YAMLish syntax: '{' at /usr/lib64/perl5/5.22.3/TAP/Parser/YAMLish/Reader.pm line 101.
Bad plan. You planned 2 tests but ran 1.
Files=1, Tests=1, 0 wallclock secs ( 0.01 usr + 0.00 sys = 0.01 CPU)
Result: FAIL
$ prove --version
TAP::Harness v3.35_01 and Perl v5.22.3
but node-tap [3] parses it fine:
$ tap test/yaml/test
test/yaml/test ........................................ 2/2
total ................................................. 2/2
2 passing (32.15ms)
ok
$ tap --version
11.0.0
[1]: https://testanything.org/tap-version-13-specification.html#yaml-blocks
[2]: http://www.yaml.org/spec/1.2/spec.html
$ curl -s http://www.yaml.org/spec/1.2/spec.html | grep -B1 JSON | head -n2
The primary objective of this revision is to bring YAML into compliance
with JSON as an official subset. YAML 1.2 is compatible with 1.1 for
[3]: http://www.node-tap.org/
So folks who are comfortable with the additional dependency can get prove-compatible output. Michael wants to stick with prove for tap-go because it is widely installed [1], but folks using node-tap or other consumers that can handle the JSON subset of YAML probably don't want the external dependency. Folks who want yaml.v2 can enable the yaml build tag [2]. Folks who are ok with JSON don't have to set any build tags. The go-yaml dependency is the only Go producer listed in [3]. There may be others, I haven't checked. The Makefile changes include new uses of wildcards [4] to pick up test/%/*.go siblings. And I'm using the stem variable $* [5] in the rule to pick up the whole test package (and not just main.go). I'm not sure how Michael wants vendoring to work. For the moment, I've softened the 'GOPATH =' to a 'GOPATH ?=' and installed the package in my local GOPATH. It's possible that we want to stick with 'GOPATH =' and drop the package under gopath/ (via a Git submodule?). [1]: mndrix/tap-go#7 (comment) [2]: https://golang.org/pkg/go/build/#hdr-Build_Constraints [3]: http://yaml.org/ [4]: https://www.gnu.org/software/make/manual/html_node/Wildcard-Examples.html [5]: https://www.gnu.org/software/make/manual/html_node/Automatic-Variables.html
* yaml: Add 'yaml' build tag for conditionally using gopkg.in/yaml.v2 Add YAML() for generating YAML blocks
|
@thaJeztah shall I perhaps do the function call replacement on the same PR or would you prefer it to be on a separate PR? |
|
Thanks! I see this PR does not include the
I think it's good to add those commits here;
- package tap // import "github.com/mndrix/tap-go"
+ package tap
Other enhancements after that could be done in a follow-up For the failing DCO sign-off, I think in the past we got an exemption from the legal team at CNCF, but it's probably worth checking if that's needed. |
Signed-off-by: Muyassarov, Feruzjon <[email protected]>
Signed-off-by: Muyassarov, Feruzjon <[email protected]>
Signed-off-by: Muyassarov, Feruzjon <[email protected]>
fmuyassarov
force-pushed
the
integrate_tap_go
branch
from
0b42308 to
cba200f
Compare
Thanks for the suggestions @thaJeztah. I have addressed above comments. Regarding DCO, would you mind to share who should I contact from the CNCF team? |
|
ping @thaJeztah |
|
Hi @thaJeztah . Please take a look when you have some time. Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Migrate the code of github.com/mndrix/tap-go as internal package with commit history as discussed in #766 (comment)
Fixes subset of the dependency issues described in #758.
I followed @thaJeztah suggestion of using git-filter-repo tool not to loose the git commit history while migrating the code and took the following steps.
As the next step, I will replace function calls of the tap-go and remove imports of github.com/mndrix/tap-go as an external package.