Refactor tests by sevki · Pull Request #26 · google/go-jsonnet

sevki · 2017-08-16T22:22:57Z

many of the demo examples on jsonnet.org do not evaluate, this PR proposes
switching the test style from a table driven tests approach to the golden files
technique that more closely mirrors the behavior of the standard library so
demo examples can be added to the test suite when go-jsonnet reaches
parity with it's c++ counterpart or earlier for individuals wanting to use a
test driven development approach.

using `t.Run` instead including the test name in error message is a more idiomatic way of testing things in go. Since this feature was added to go in 1.7 release and the travis config explicitly specifies 1.4 and 1.5 (2 and 2,5 year old releases) as test targets this change will break the CI builds, therefore this commit also proposes dropping those releases in favour of adding a newer version of go (1.8) as the test target. Signed-off-by: Sevki <s@sevki.org>

since non of the test cases actually use the err string field and the golden file testing pattern does not really need it, this commit proposes the removal of the errString field from tests Signed-off-by: Sevki <s@sevki.org>

this commit proposes using a file based golden tests approach, parts of this refactor uses code and conventions from the go std library. At it's current state go-jsonnet does not support many of the examples on http://jsonnet.org/docs/demo.html, motive behind the refactor is to add the demo examples to the test suite and the demo examples are too big to be inlined. Signed-off-by: Sevki <s@sevki.org>

sevki · 2017-08-16T22:23:36Z

to follow the std library conventions some of the test logic and code has been "borrowed" from the std library, as I am not a lawyer I don't know if it violates the license agreement of go or go-jsonnet. If they are problematic I'm happy to do a rewrite of the offending code.

coveralls · 2017-08-16T22:25:09Z

Coverage decreased (-0.02%) to 73.72% when pulling f535776 on sevki:refactor-tests into 1826830 on google:master.

sparkprime · 2017-08-17T16:32:58Z

Thanks again. Stan is working full time on developing go-jsonnet. Why don't we bring you in on our plans? We'd be very happy to work closely with you on this. ksonnet slack might be a good place to sync up.

This is a good start on improving end-to-end testing. For testing going forward, it'd be great to have:

A unified suite of test inputs/outputs for Go and C++
A single tool that can work its way over the test suite, providing a summary of test successes / failures (this is currently done with a bash script in C++ but we could retire that). This could work by having the test tool be written in Go, and call the C++ Jsonnet implementation via the cgo wrapper.

sparkprime · 2017-08-17T16:44:21Z

Collapsing of lots of tiny tests into some sort of single file that describes both input and golden for many tests. Options for the format of this file include: a) Jsonnet itself (text blocks), b) YAML (block style), c) A single text file with special comments that divide the file into multiple inputs / goldens. A disadvantage of (a) is that quite a lot of Jsonnet functionality has to work before the tests will even run. A disadvantage of both (a) and (b) are that editors' Jsonnet syntax highlighting will not work because the whole test snippet would be rendered as a string. (c) would work by doing a simple pre-pass on the file to chop it up into valid Jsonnet snippets. It would not be valid Jsonnet code before that, but it would be close enough for syntax highlighting to work.

What do you think?

sbarzowski · 2017-08-17T17:13:43Z

This looks like a step in good direction. Thank you!

Here is more info if you're interested:

many of the demo examples on jsonnet.org do not evaluate

Yeah. Majority of language features are still missing and I'm working on changing that. There's an uncomfortably large PR (#24) that adds most missing language features - but there's much more work left to do.

It's currently possible to run tests from C++ version (tests.sh). It also uses a golden files pattern (take a look: https://github.com/google/jsonnet/tree/master/test_suite). It also runs all the examples from the website. Ideally we would have one way to run end2end tests against any implementation and one common, unified test suite. And as the test suite logic gets more complex having it implemented in Go seems like a better option than bash we have now.

There are a few reasons why we have these tests in go version:
a) Historical. Not so long ago, there was no commandline in go version and C++ tests hardcoded the paths so it was just the only way.
b) Tests in C++ version are not very granular. They are more about suitable for making sure all boundary cases are covered.
c) Related to the previous point, the tests are not very partial implementation friendly. It would be nice to have tests of each feature which don't unnecessarily depend on other stuff (like the tests in Go version). In particular it would be nice if we had tests for basic constructs that doesn't depend on a standard library (C++ uses assertEquals to test even very basic stuff - and for a good reason we would have hundreds of golden files and navigating would be much harder).
d) It's convenient to keep track of tests that are supposed to pass and only run these most of the time :-). It's hard to have TDD style workflow while being spammed with a few thousand lines of irrelevant stack traces.

As you can see, none of these is a fundamental problem.

There are also some other practical problems with tests:
a) How to test stack traces, errors, autoformatter etc. where subtle differences between implementations are sometimes hard to avoid.
b) Loading stdlib every time for every little test. We're parsing a 1000 lines file to execute one line. It would make tests much faster if we did it once for a bunch of tests, at least by default. This is more tricky when we're actually running the commandline interpreter line in C++ version.
c) License boilerplate etc. may be annoying for granular tests in separate files.

Note: What I said applies to end2end tests. We also have tests for specific components, but that's a different discussion.

sbarzowski · 2017-08-17T17:14:51Z

(uh, apparently I had some race condition with @sparkprime, but it seems our message is mostly consistent :-)).

sparkprime · 2017-08-17T19:26:50Z

main_test.go

+	}
+}
+
+// "barrowed" from the std library


s/barrowed/borrowed

Can you add to the top of this file, underneath the existing copyright notice, the Go one: https://github.com/golang/go/blob/master/LICENSE

If we substantially rewrite it then we can remove it later.

Or alternatively, move the diff code into its own file and use that copyright header for it.

@sparkprime how would you feel about adding a library that does the diffing?
since it's in the tests, it's not a dependency and it would look nicer and would resolve the license issue

diff:

deps:

Details

sevki@sevki-ThinkPad-X220:~/code/go/src/github.com/google/go-jsonnet$ go list -f '{{ join .Imports "\n" }}' bytes encoding/hex fmt math reflect sort strconv strings unicode/utf8 sevki@sevki-ThinkPad-X220:~/code/go/src/github.com/google/go-jsonnet$ go list -f '{{ join .Deps "\n" }}' bytes encoding/hex errors fmt internal/cpu internal/poll internal/race io math os reflect runtime runtime/internal/atomic runtime/internal/sys sort strconv strings sync sync/atomic syscall time unicode unicode/utf8 unsafe

FYI for pretty diffs, we're using git diff in C++/bash version.

Can you post a link to this lib?

yup. this one https://github.com/sergi/go-diff/

A library to do it sounds good to me. It removes the unix dependency (hopefully).

yeah, there is that :)

the proposed change would remove the dependency of a diff binary in favour of using a library Signed-off-by: Sevki <s@sevki.org>

coveralls · 2017-08-17T22:23:29Z

Coverage decreased (-0.02%) to 73.72% when pulling 7b875bd on sevki:refactor-tests into 1826830 on google:master.

sparkprime · 2017-08-18T01:14:48Z

LGTM, ready to go?

sevki · 2017-08-20T11:19:57Z

I'm good, if you guys want to move forward with this

sevki added 3 commits August 16, 2017 23:03

remove errString from main_test.go

4582b1c

since non of the test cases actually use the err string field and the golden file testing pattern does not really need it, this commit proposes the removal of the errString field from tests Signed-off-by: Sevki <s@sevki.org>

googlebot added the cla: yes label Aug 16, 2017

sparkprime reviewed Aug 17, 2017

View reviewed changes

change the diff function

7b875bd

the proposed change would remove the dependency of a diff binary in favour of using a library Signed-off-by: Sevki <s@sevki.org>

sbarzowski approved these changes Aug 18, 2017

View reviewed changes

sparkprime merged commit 8c86686 into google:master Aug 23, 2017

sbarzowski mentioned this pull request Sep 19, 2017

Improve imports #66

Merged

Conversation

sevki commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sevki commented Aug 16, 2017

Uh oh!

coveralls commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sparkprime commented Aug 17, 2017

Uh oh!

sparkprime commented Aug 17, 2017

Uh oh!

sbarzowski commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbarzowski commented Aug 17, 2017

Uh oh!

sparkprime Aug 17, 2017

Choose a reason for hiding this comment

Uh oh!

sparkprime Aug 17, 2017

Choose a reason for hiding this comment

Uh oh!

sevki Aug 17, 2017

Choose a reason for hiding this comment

Uh oh!

sbarzowski Aug 17, 2017

Choose a reason for hiding this comment

Uh oh!

sevki Aug 17, 2017

Choose a reason for hiding this comment

Uh oh!

sparkprime Aug 17, 2017

Choose a reason for hiding this comment

Uh oh!

sevki Aug 17, 2017

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sparkprime commented Aug 18, 2017

Uh oh!

sevki commented Aug 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sevki commented Aug 16, 2017 •

edited

Loading

coveralls commented Aug 16, 2017 •

edited

Loading

sbarzowski commented Aug 17, 2017 •

edited

Loading

coveralls commented Aug 17, 2017 •

edited

Loading