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.

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

Configure matcher to record_errors #52

Merged
merged 1 commit into from
Feb 24, 2017
Merged

Conversation

seanpdoyle
Copy link
Collaborator

@seanpdoyle seanpdoyle commented Feb 24, 2017

Closes #51.

By default, JsonMatchers.configuration.options[:record_errors] = true.

Extract JsonMatchers::Validator to encapsulate error reporting
behavior.

JSON::Validator exposes a #validate! method that will raise on
validation failures, and a #fully_validate! method that won't raise,
but will return an array of error messages instead.

Additionally, this commit introduces the #with_options test helper
that scopes configuration options to a given test block.


expect(response_for({ "id" => 1, "title" => "bar" })).
to match_response_schema("foo_schema")
expect(response_for({ "id" => 1 })).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant curly braces around a hash parameter.

},
})

expect(response_for({ "id" => 1, "title" => "bar" })).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant curly braces around a hash parameter.

"id" => { "type" => "number" },
"title" => { "type" => "string" },
},
})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent the right brace the same as the first position after the preceding left parenthesis.

config.options.delete(:strict)
with_options(strict: true) do
create_schema("foo_schema", {
"type" => "object",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

JsonMatchers.configure do |config|
config.options.delete(:strict)
with_options(strict: true) do
create_schema("foo_schema", {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant curly braces around a hash parameter.

@seanpdoyle seanpdoyle force-pushed the refactor-bbonifield-pr branch 4 times, most recently from 2c686ee to ee46320 Compare February 24, 2017 17:54
})
invalid_payload = response_for({ "username" => "foo" })

expect {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using {...} for multi-line blocks.

}
}
})
invalid_payload = response_for({ "username" => "foo" })

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant curly braces around a hash parameter.

]
}
}
})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent the right brace the same as the first position after the preceding left parenthesis.

{ "minLength": 5 }
]
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last item of a multiline hash.

{ "type": "string" },
{ "minLength": 5 }
]
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last item of a multiline hash.

"allOf": [
{ "type": "string" },
{ "minLength": 5 }
]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last item of a multiline hash.

"username" => {
"allOf": [
{ "type": "string" },
{ "minLength": 5 }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last item of a multiline array.


expect(response_for({})).not_to match_response_schema("foo_schema")
create_schema("foo_schema", {
"type" => "object",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

"required" => ["foo"])

expect(response_for({})).not_to match_response_schema("foo_schema")
create_schema("foo_schema", {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant curly braces around a hash parameter.

})
invalid_payload = response_for({ "username" => "foo" })

expect {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using {...} for multi-line blocks.

}
}
})
invalid_payload = response_for({ "username" => "foo" })

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant curly braces around a hash parameter.

]
}
}
})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indent the right brace the same as the first position after the preceding left parenthesis.

{ "minLength": 5 }
]
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last item of a multiline hash.

{ "type": "string" },
{ "minLength": 5 }
]
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last item of a multiline hash.

"allOf": [
{ "type": "string" },
{ "minLength": 5 }
]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last item of a multiline hash.

"username" => {
"allOf": [
{ "type": "string" },
{ "minLength": 5 }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a comma after the last item of a multiline array.


expect(response_for({})).not_to match_response_schema("foo_schema")
create_schema("foo_schema", {
"type" => "object",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.

"required" => ["foo"])

expect(response_for({})).not_to match_response_schema("foo_schema")
create_schema("foo_schema", {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant curly braces around a hash parameter.

Closes [#51].

By default, `JsonMatchers.configuration.options[:record_errors] = true`.

Extract `JsonMatchers::Validator` to encapsulate error reporting
behavior.

`JSON::Validator` exposes a `#validate!` method that will raise on
validation failures, and a `#fully_validate!` method that won't raise,
but will return an array of error messages instead.

[#51]: #51
@seanpdoyle seanpdoyle force-pushed the refactor-bbonifield-pr branch from ee46320 to ee1349d Compare February 24, 2017 18:04
@seanpdoyle seanpdoyle merged commit ee1349d into master Feb 24, 2017
@mike-burns
Copy link

Can you clarify why this should be configurable?

@seanpdoyle seanpdoyle deleted the refactor-bbonifield-pr branch February 24, 2017 18:10
@seanpdoyle
Copy link
Collaborator Author

Can you clarify why this should be configurable?

json_matchers supports configuring the underlying JSON::Validator.

By default, the underlying Validator will raise an error when passed invalid JSON. Unfortunately, this error tends to be high-level and vague.

When configured with record_errors: true, the validator will collect and return an array of error messages instead of raising.

This feels like a reasonable default, so this PR makes it so.

@seanpdoyle
Copy link
Collaborator Author

Your question raises a good point: since it's now the default, should we support record_errors: false?

Before this set of PRs, we'd forward along the configuration options (like strict: true), unchanged, to the underlying JSON::Validator. An option's value didn't affect the json_matchers implementation. In the case of record_errors, it changes the Validator to return an array instead of raising on invalid input, so we have to modify our implementation to account for that.

Since it is now the default, do you think we should explicitly remove support for record_errors: false?

@mike-burns
Copy link

Since it is now the default, do you think we should explicitly remove support for record_errors: false?

I think so? Or: when would I ever set it to false?

@seanpdoyle
Copy link
Collaborator Author

when would I ever set it to false?

I am not sure of that.

As a solution, we could outright ignore the record_errors option, mention so in the README, and deliberately remove it from any user-configured options.

What do you think?

@mike-burns
Copy link

+1 to removing this as configuration. I want my matcher to always record errors.

seanpdoyle added a commit that referenced this pull request Feb 24, 2017
Ensure that validation is always configured with  `record_errors: true`,
based on the following [comments]:

> By default, the underlying Validator will raise an error when passed
> invalid JSON. Unfortunately, this error tends to be high-level and
> [vague](#51).

> When configured with `record_errors: true`, the validator will collect
> and return an array of error messages instead of raising.

> This feels like a [reasonable
> default](#51 (comment)),
> so this PR makes it so.

[comments]: #52 (comment)
@seanpdoyle
Copy link
Collaborator Author

@mike-burns #54

seanpdoyle added a commit that referenced this pull request Feb 24, 2017
Ensure that validation is always configured with  `record_errors: true`,
based on the following [comments]:

> By default, the underlying Validator will raise an error when passed
> invalid JSON. Unfortunately, this error tends to be high-level and
> [vague](#51).

> When configured with `record_errors: true`, the validator will collect
> and return an array of error messages instead of raising.

> This feels like a [reasonable
> default](#51 (comment)),
> so this PR makes it so.

[comments]: #52 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants