Report status code before schema #338
Conversation
| @@ -7,13 +7,13 @@ | |||
| }, { | |||
| "scenario" : "disable request validation", | |||
| "stepsSummary" : { | |||
| "numberOfFailed" : 2 | |||
| "numberOfSuccessful" : 3 | |||
There was a problem hiding this comment.
do we still need a failed case?
| @@ -982,6 +983,75 @@ class HttpGroovyTest implements HttpConfiguration { | |||
| http.lastValidationResult.errorMessage.should == ~/java.lang.IllegalArgumentException: Request header is null/ | |||
| } | |||
|
|
|||
| @Test | |||
| void "implicit status code mismatch reported before additional validators"() { | |||
| HttpValidationHandlers.register { result -> throw new AssertionError((Object)"schema validation error") } | |||
There was a problem hiding this comment.
wonder if we should use the withAdditional... pattern we use for some other handler like classes
| } | ||
|
|
||
| @Test | ||
| void "assertion and additional validation errors"() { |
There was a problem hiding this comment.
style of the test name mismatches the other ones.
|
|
||
| @Test | ||
| void "additional validator errors reported if status code is correct"() { | ||
| HttpValidationHandlers.register { result -> throw new AssertionError((Object)"schema validation error") } |
There was a problem hiding this comment.
if (as per other comment) we don't introduce a withAdditional like method, then maybe we should extract the repetitive code in this test to a private static method wtihAdditionalFailedHandler('schema validation error') {...}
| statusCode.should == 404 | ||
| id.should == 'foo' | ||
| } | ||
| } should throwException(AssertionError, ~/expected: "foo"/) |
There was a problem hiding this comment.
not clear by this check if the schema validation error is not part of the exception.
| @@ -23,6 +23,15 @@ | |||
| public class HttpValidationHandlers { | |||
| private static final List<HttpValidationHandler> configurations = ServiceLoaderUtils.load(HttpValidationHandler.class); | |||
|
|
|||
| public static void register(HttpValidationHandler handler) { | |||
There was a problem hiding this comment.
Most of the times I call these methods simply add. But I may have slipped a few ones like register. Should we rename to add?
| configurations.add(handler); | ||
| } | ||
|
|
||
| public static void reset() { |
There was a problem hiding this comment.
Would prefer to maintain as fewer resets like methods as possible.
| try { | ||
| code { | ||
| http.get("/notfound") {} | ||
| } should throwException(AssertionError, ~/(?s)header.statusCode.*404.*200/) |
There was a problem hiding this comment.
order is not clear based on the assertion
|
I think I've addressed all your comments @MykolaGolubyev , please review again. |
|
beautiful! |
|
I wondered about that and then forgot to check. Looks safe to remove though, commit coming up. |
Closes #302
Before merging this, I'd like some feedback. Prior to this PR, any
HttpValidationHandlererrors are reported even if status code fails or user expectations fail. This is confusing in the case where you expect a request to succeed but it fails and the failure response is of a different schema to what you'd expect; it's confusing because you get a schema validation failure and the fact the request failed is not reported.We have two options. The behaviour this PR introduces is to only run
HttpValidationHandlersif everything else is fine. So generally you'll get a status code error or user expectation error and only if both of those are fine will you get a schema error.Do you think it would be better to report in this order:
I suspect that's generally better because if the schema is incorrect the odds of the user expectations passing are reduced anyway and could send someone down a rabbit hole. Unfortunately, this probably requires substantially more refactoring.
Thoughts?