Bugfix Json Schema, Improve json schema tests #1015

simitt · 2018-06-19T09:39:26Z

Fix bugs uncovered with improved tests
Do automatic validation for keyword limitations
Remove files for invalid payloads, instead create invalid payloads out of valid payload
Allow to define simple and conditional rules for testing payloads.

This PR is based on updating github.com/santhosh-tekuri/jsonschema #1013 and on removing archived library github.com/fatih/set #1014.

simitt · 2018-06-19T11:06:20Z

docs/spec/transactions/transaction.json

@@ -50,8 +50,7 @@
            "regexProperties": true,
            "patternProperties": {
                "^[^.*\"]*$": {
-                    "$ref": "mark.json",
-                    "maxLength": 1024


This length restriction did not make sense, as it doesn't refer to the properties key but the value and the value is a mark.

graphaelli

some comments collected so far, still working through parts of the pull request

graphaelli · 2018-06-21T19:24:36Z

processor/sourcemap/package_tests/json_schema_test.go

+
+	for _, d := range testData {
+		data, err := loader.LoadData(d.File)
+		assert.Nil(t, err)


assert.NoError will format falied assertions nicely. This should also continue or otherwise stop working with data if err != nil (or more handy: ! assert.NoError), otherwise it will try to validate invalid data even though the test already failed.

WDYT about introducing testify/require, it implements the same interface as testify.assert but fails fast. I think this would be perfect for assertions that should abort the whole test, like this.

graphaelli · 2018-06-21T19:37:24Z

tests/json_schema.go

+	Absence []string
+	// If requirements for a field apply in case of
+	// anothers key specific values, add the key and its values.
+	Existance map[string]interface{}


graphaelli · 2018-06-21T19:54:27Z

tests/json_schema.go

 )

+type JsonSchemaTestData struct {
+	Proc             processor.Processor
+	ValidPayloadPath string


Maybe a comment here too? I'm not clear on whether any valid payload will do or minimal or fully filled in? Probably will figure out during this review but would be handy for new devs.

graphaelli · 2018-06-21T19:56:02Z

tests/json_schema.go

+	))
+
+	payload, err := loader.LoadData(td.ValidPayloadPath)
+	assert.NoError(t, err, fmt.Sprintf("File %s not loaded", td.ValidPayloadPath))


should exit early if ! assert.NoError

graphaelli · 2018-06-21T19:56:23Z

tests/json_schema.go

+func (td *JsonSchemaTestData) testKeywordLimitation(t *testing.T) {
+	// fetch keyword restricted field names from ES template
+	keywordFields, err := fetchFlattenedFieldNames(td.LoadTemplatePaths, addKeywordFields)
+	assert.NoError(t, err)


graphaelli · 2018-06-21T19:56:42Z

tests/json_schema.go

+
+	// load payload
+	payload, err := loader.LoadData(td.ValidPayloadPath)
+	assert.NoError(t, err, fmt.Sprintf("File %s not loaded", td.ValidPayloadPath))


graphaelli · 2018-06-21T19:56:47Z

tests/json_schema.go

+		payload = iterateMap(payload, "", fnKey, keyToChange, val, upsertFn).(obj)
+	}
+	err = td.Proc.Validate(payload)
+	assert.NoError(t, err)


graphaelli · 2018-06-21T19:58:04Z

tests/json_schema.go

@@ -27,7 +281,8 @@ type Mapping struct {
 }

 func TestPayloadAttributesInSchema(t *testing.T, name string, undocumentedAttrs *Set, schema string) {
-	payload, _ := loader.LoadValidData(name)
+	payload, err := loader.LoadValidData(name)
+	assert.NoError(t, err)


That require is neat - missed one or still applying those?

Missed that one - changing it now.

graphaelli · 2018-06-21T20:01:50Z

tests/json_schema.go

+
+type TestData struct {
+	Key       string
+	Invalid   []interface{}


Don't we lose the checks for why the data is invalid with this? Not that comparing strings wasn't great but it was actually useful

Very valid input, I'll come up with something.

simitt · 2018-06-22T10:01:00Z

@graphaelli I'll add a rework to check for error messages and improve the assertions. I'll close the PR for now to avoid the noise, and reopen later.

simitt · 2018-06-22T13:19:11Z

I incorporated your suggestions @graphaelli:

fail fast when assertion fails for precondition
add expected error messages to validation failure checks
add more comments

graphaelli · 2018-06-23T15:57:17Z

Makefile

-	@cp tests/data/valid/transaction/minimal_payload.json docs/data/intake-api/generated/transaction/
-	@cp tests/data/valid/transaction/minimal_span.json docs/data/intake-api/generated/transaction/
-	@cp tests/data/valid/sourcemap/bundle.js.map docs/data/intake-api/generated/sourcemap/
+	@cp tests/data/error/payload.json docs/data/intake-api/generated/error/


If we're moving things around anyway, how about tests/data -> testdata to give the go tool less to look at? This would take some work as sometimes we refer to paths from within the tests directory, sometimes from top level package path.

I like the idea, will add it in an extra commit.

graphaelli · 2018-06-23T16:01:12Z

tests/json_schema.go

+type ProcessorSetup struct {
+	Proc processor.Processor
+	// path to payload that should be a full and valid example
+	FullPayloadPath string


graphaelli · 2018-06-23T16:18:19Z

tests/json_schema.go

+	Invalid   Invalid
+	Condition Condition
+}
+type Invalid = map[string][]interface{}


It took me a little bit to understand what is happening here - I think this could be a more specific structure that captures msg and values and then include a []Invalid in SchemaTestData - wdyt?

good idea, changed

graphaelli · 2018-06-23T16:19:12Z

tests/json_schema.go

@@ -27,7 +281,8 @@ type Mapping struct {
 }

 func TestPayloadAttributesInSchema(t *testing.T, name string, undocumentedAttrs *Set, schema string) {
-	payload, _ := loader.LoadValidData(name)
+	payload, err := loader.LoadValidData(name)
+	assert.NoError(t, err)


That require is neat - missed one or still applying those?

graphaelli · 2018-06-23T16:23:52Z

Nitpicking - is the 80 character wrapping new? It seems strange that some lines are wrapped at 80 and others at 120 - I think they should all be at 120.

simitt · 2018-06-25T09:01:16Z

Thanks for the review @graphaelli. I liked your suggestions and pushed the changes.

graphaelli

Really nice work

* Fix bugs uncovered with improved tests * Do automatic validation for keyword limitations * Remove files for invalid payloads, instead create invalid payloads out of valid payload * Allow to define simple and conditional rules for testing payloads.

roncohen · 2018-06-27T16:18:00Z

needs a backport to 6.x ?

simitt requested a review from graphaelli June 19, 2018 09:39

simitt force-pushed the improve-json-spec-tests branch from ff076ff to 9194d6c Compare June 19, 2018 11:05

simitt commented Jun 19, 2018

View reviewed changes

simitt force-pushed the improve-json-spec-tests branch from 9194d6c to 0002caa Compare June 20, 2018 07:32

graphaelli reviewed Jun 21, 2018

View reviewed changes

simitt closed this Jun 22, 2018

simitt reopened this Jun 22, 2018

graphaelli reviewed Jun 23, 2018

View reviewed changes

graphaelli approved these changes Jun 25, 2018

View reviewed changes

alvarolobato added the [zube]: In Progress label Jun 25, 2018

simitt added 2 commits June 25, 2018 17:18

Move test data into testdata folder

e0c28ef

simitt force-pushed the improve-json-spec-tests branch from d00f064 to e0c28ef Compare June 25, 2018 15:18

simitt merged commit 8a7016a into elastic:master Jun 25, 2018

zube bot added [zube]: Done and removed [zube]: In Progress labels Jun 25, 2018

simitt deleted the improve-json-spec-tests branch June 28, 2018 11:21

simitt mentioned this pull request Jun 28, 2018

catch errors when starting the beater #1045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix Json Schema, Improve json schema tests #1015

Bugfix Json Schema, Improve json schema tests #1015

simitt commented Jun 19, 2018 •

edited by zube bot

Loading

simitt Jun 19, 2018

graphaelli left a comment

graphaelli Jun 21, 2018

simitt Jun 22, 2018

graphaelli Jun 21, 2018

graphaelli Jun 21, 2018

graphaelli Jun 21, 2018

graphaelli Jun 21, 2018

graphaelli Jun 21, 2018

graphaelli Jun 21, 2018

graphaelli Jun 21, 2018

graphaelli Jun 23, 2018

simitt Jun 25, 2018

graphaelli Jun 21, 2018 •

edited

Loading

simitt Jun 22, 2018

simitt commented Jun 22, 2018

simitt commented Jun 22, 2018

graphaelli Jun 23, 2018

simitt Jun 25, 2018

graphaelli Jun 23, 2018

graphaelli Jun 23, 2018

simitt Jun 25, 2018

graphaelli Jun 23, 2018

graphaelli commented Jun 23, 2018

simitt commented Jun 25, 2018

graphaelli left a comment

roncohen commented Jun 27, 2018

Bugfix Json Schema, Improve json schema tests #1015

Bugfix Json Schema, Improve json schema tests #1015

Conversation

simitt commented Jun 19, 2018 • edited by zube bot Loading

Choose a reason for hiding this comment

graphaelli 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

Choose a reason for hiding this comment

graphaelli Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simitt commented Jun 22, 2018

simitt commented Jun 22, 2018

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

graphaelli commented Jun 23, 2018

simitt commented Jun 25, 2018

graphaelli left a comment

Choose a reason for hiding this comment

roncohen commented Jun 27, 2018

simitt commented Jun 19, 2018 •

edited by zube bot

Loading

graphaelli Jun 21, 2018 •

edited

Loading