Table tests (initial code) #46

smyrman · 2016-09-16T07:53:28Z

Pushing for an initial review to check that I am moving in the right direction. Note that this PR is based on #45, so we should probably either merge that first, or abandon it and review everything here. @rs, up to you.

I was aiming to move in a similar direction as the schema package by having one test file per Validator type. Even there is no file per validator for the encoding part yet (maybe there will be in the future to prevent validatorToJSONSchema from growing to large?), it made it easier to see what I have implemented tests for, and to splitt concerns...

smyrman · 2016-09-16T08:11:46Z

Tested Go 1.6 code-path and pushed an amended commit to the branch:

$ docker run -v $GOPATH:/go golang:1.6 go test -v github.com/rs/rest-layer/schema/encoding/jsonschema
...
ok      github.com/rs/rest-layer/schema/encoding/jsonschema 0.019s

smyrman

Some changes done to non-tests.

smyrman · 2016-09-16T08:15:50Z

schema/float_test.go

+	assert.Equal(t, 3.1, s)
+	s, err = Float{Boundaries: &Boundaries{Min: 2, Max: math.NaN()}}.Validate(3.1)
+	assert.NoError(t, err)
+	assert.Equal(t, 3.1, s)


Not sure if support for NaN and Inf values was intended to work, but since it does work, I added some tests for it.

smyrman · 2016-09-16T08:16:22Z

schema/integer_test.go

+	assert.Equal(t, 3, s)
+	s, err = Integer{Boundaries: &Boundaries{Min: 2, Max: math.NaN()}}.Validate(3)
+	assert.NoError(t, err)
+	assert.Equal(t, 3, s)


Same here. Not sure if support for NaN and Inf values was intended to work, but since it does work, I added some tests for it.

smyrman · 2016-09-16T08:19:03Z

schema/encoding/jsonschema/jsonschema.go

+		ew.writeFormat(`, "maximum": %s`, strconv.FormatFloat(b.Max, 'E', -1, 64))
+	}
+	return ew.err
+}


I added this method to use the same code-path for encoding both integer and float boundaries. I also added support for NaN and Inf values so that:

Max=NaN is treated as no limit.

Max=+Inf is treated as no limit.

Min=NaN is treated as no limit.

Min=-Inf is treated as no limit.

Might be useful in some cases to only set one limit... I checked that this worked with the existing schema package logic by first inspecting the code, and then by adding some tests.

Hint: The following is always true for NaN, where f is any float64 value:

(f < NaN) == false

(f > NaN) == false

hmm, isn't NaN an error?

NaN means Not a Number

Yes but why accept it? I mean, nan is triggered by strings, so is it a good idea to silently accept it?

I mean, nan is triggered by strings

What do you mean it's triggered by strings? Where, and how? E.g strconv.ParseFloat() would return an error and a zero value, not NaN: https://play.golang.org/p/Xx50p-LgL7

so is it a good idea to silently accept it?

I don't think there are too many ways people can set NaN by mistake. They may get an NaN if they do math.Acos(1.1) etc., but that doesn't really sound like a common pattern for setting float limits? I would assume the most reasonable way people could set a NaN value, would be if they did so deliberately via math.NaN()...

Yes but why accept it?

Currently I made the JSON Schema accept it simply because the schema package already accepts it (silently), and we should handle the value somehow....

PS! This doesn't mean it can't be changed, if there is a compelling reason to change how NaN is handled by the schema package, but I suggest that's handled in another PR, and only if needed?

rs · 2016-09-16T17:25:50Z

Can you please rebase on master?

smyrman · 2016-09-16T18:21:41Z

rebased

smyrman · 2016-09-16T18:33:18Z

schema/encoding/jsonschema/all_test.go

+			// "description". Should it be encoded as "description" here as well?
+			expect: `{
+				"type": "object",
+				"title": "A list of students",


I am wondering if we should switch this to "description". @yanfali, do you have an opinion? Is it any reason in particular that you chose to encode the schema description as "title" for "object", while it's encoded as "description" for other types?

JSON Schema defines both "title" and "description", where title is supposed to be shorter, and description is supposed to be longer. http://json-schema.org/latest/json-schema-validation.html#anchor98

I think the best thing to do would be to switch to "description", and if we need "title" in the future, add a Title property to schema.Field.

I don't feel strongly about this. In fact the first time I submitted this, I totally forgot I used title.

In which case I suggest we change it to description for consistency :-)

smyrman · 2016-09-17T08:58:44Z

Added float tests, added one more "all" (package level) test, and removed redundant tests from old test file.

smyrman · 2016-09-17T09:02:46Z

schema/encoding/jsonschema/jsonschema_test.go

-	assert.JSONEq(t, expect, b.String())
-}
-
-func TestDefaultEncodingWithStringFieldAndIntegerDefault(t *testing.T) {


OK, haven't added any tests for default, so this was removed by mistake. Will add some tests to replace it.

yanfali · 2016-09-17T20:11:11Z

Sorry I've been tied up at work. I will review it this evening PST and let you know my thoughts. Thanks for doing this.

smyrman · 2016-09-19T11:32:45Z

Amended to include tests for Default.

smyrman · 2016-09-19T11:33:23Z

Sorry I've been tied up at work.

No worries:-)

yanfali

OK, initial review done. A few comments, that's a lot of work thanks. Only major concern is package name. I made a similar choice when I created the jsonschema package, so make sure it runs from the top most level too. Putting it in subpath test/ would probably work just as well.

yanfali · 2016-09-19T17:07:24Z

schema/encoding/jsonschema/all_go16_test.go

@@ -0,0 +1,11 @@
+// +build !go1.7
+
+package jsonschema_test


interesting, why does this have a separate package? I'm not familiar with this pattern. Doesn't this mean you can't inspect package variables? Is that the intention? what happens when you run this from the top most level via ./... ?

@yanfali, This is the Go way to test the public interface of your package. It makes you having to import the package you are testing, and only give access to the public interface. Presumably you could have done the same for the tests you put in the encoding package.

Excellent, thanks for explaining. The encoding test was testing imports rather than public interface so the intention was different. I was ignorant of this pattern.

I was hoping to link to a blog post or some docs explaining the "_test" suffix, but I couldn't find it just now. However, it's a very common pattern, and you can also see it's commonly used in the Go standard library. E.g. https://golang.org/src/strings/reader_test.go

yanfali · 2016-09-19T17:09:53Z

schema/encoding/jsonschema/all_test.go

+}
+
+// test performs the actual test and is used by both implementations of run.
+func (tc *encoderTestCase) test(t *testing.T) {


yanfali · 2016-09-19T17:15:22Z

schema/encoding/jsonschema/all_test.go

+			schema: schema.Schema{
+				Fields: schema.Fields{
+					"name": schema.Field{
+						Default: map[string]interface{}{


ok, this is an odd use case, is this the same as allowed?

Yeah, just documenting the current behaviour. I adde a comment some way up (above the test where I use an int).

I will change it to be less odd to avoid confusion, probably by setting the default one level out.

yanfali · 2016-09-19T17:28:10Z

schema/encoding/jsonschema/jsonschema.go

+		ew.writeFormat(`, "maximum": %s`, strconv.FormatFloat(b.Max, 'E', -1, 64))
+	}
+	return ew.err
+}


hmm, isn't NaN an error?

smyrman · 2016-09-20T07:24:05Z

hmm, isn't NaN an error?

In many programs it probably is, but for Go it's a IEEE 754 “not-a-number” value. I found it "fitting" as a way of describing "no limit", although it's not strictly needed in our case as we could rely on +/-Inf as the only "supported" values. However, without any added special-casing, both NaN and Inf numbers would result in invalid JSON if specified by the user.

smyrman · 2016-09-20T07:29:46Z

Only major concern is package name. I made a similar choice when I created the jsonschema package, so make sure it runs from the top most level too. Putting it in subpath test/ would probably work just as well.

As mentioned, adding a "_test" suffix to the package name for any test that do not require access to private types, functions or variables, is the Go way. Also, it's the Go way to keep all package tests in the same folder/package so that e.g. "go test -cover IMPORT_PATH" never lies;-)

smyrman · 2016-09-22T22:16:17Z

Latest change include some encoding changes:

Renamed "title" -> "description" for consistency on object.
Set "additionalProperties".

Test changes:

Moved string tests from old test file to string_test.go
Reworked float/integer tests to compare only a sub-set of the JSON (The same way string_test.go now does it...).

rs · 2016-09-23T17:04:27Z

@smyrman: Are you ready for merging?

smyrman · 2016-09-23T22:09:33Z

@rs, yes, I think this PR can be merged.

rs · 2016-09-24T02:23:08Z

Could you please rebase on master?

Adds some initial table tests, and remove corresponding tests from the previous test implementation. Also updates the boundary handling encoding wise to handle NaN and Inf values gracefully. In the schema package, the integer and float FieldValidators should already handle this values with existing code. Added tests to make sure it continues to do so.

smyrman · 2016-09-24T14:58:06Z

@rs, it's now rebased again.

smyrman mentioned this pull request Sep 16, 2016

[jsonschema] Table driven tests #44

Closed

smyrman force-pushed the table-tests branch from 837ce35 to f9214a3 Compare September 16, 2016 08:07

smyrman commented Sep 16, 2016

View reviewed changes

smyrman force-pushed the table-tests branch from f9214a3 to 5da5f04 Compare September 16, 2016 18:20

smyrman commented Sep 16, 2016

View reviewed changes

smyrman force-pushed the table-tests branch from 5da5f04 to 2b248bb Compare September 17, 2016 08:57

smyrman commented Sep 17, 2016

View reviewed changes

smyrman force-pushed the table-tests branch from 2b248bb to 46a9041 Compare September 19, 2016 11:32

yanfali reviewed Sep 19, 2016

View reviewed changes

smyrman force-pushed the table-tests branch from 46a9041 to 032fae6 Compare September 22, 2016 22:11

smyrman force-pushed the table-tests branch from 032fae6 to 3d57393 Compare September 24, 2016 14:55

rs merged commit 5f71d11 into rs:master Sep 24, 2016

Table tests (initial code) #46

Table tests (initial code) #46

Conversation

smyrman commented Sep 16, 2016 • edited

smyrman commented Sep 16, 2016

smyrman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smyrman Sep 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smyrman Sep 22, 2016 • edited

Choose a reason for hiding this comment

smyrman Sep 22, 2016 • edited

Choose a reason for hiding this comment

rs commented Sep 16, 2016

smyrman commented Sep 16, 2016

smyrman Sep 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smyrman commented Sep 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanfali commented Sep 17, 2016

smyrman commented Sep 19, 2016

smyrman commented Sep 19, 2016

yanfali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smyrman commented Sep 20, 2016 • edited

smyrman commented Sep 20, 2016 • edited

smyrman commented Sep 22, 2016

rs commented Sep 23, 2016

smyrman commented Sep 23, 2016

rs commented Sep 24, 2016

smyrman commented Sep 24, 2016

smyrman commented Sep 16, 2016 •

edited

smyrman Sep 16, 2016 •

edited

smyrman Sep 22, 2016 •

edited

smyrman Sep 22, 2016 •

edited

smyrman Sep 16, 2016 •

edited

smyrman commented Sep 20, 2016 •

edited

smyrman commented Sep 20, 2016 •

edited