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

Report status code before schema #338

Merged
merged 9 commits into from May 21, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -44,13 +44,13 @@
"200": {
"description": "customer already exists",
"schema": {
"$ref": "#/definitions/Employee"
"$ref": "#/definitions/Id"
}
},
"201": {
"description": "customer created successfully",
"schema": {
"$ref": "#/definitions/Employee"
"$ref": "#/definitions/Id"
}
}
}
Expand All @@ -72,6 +72,17 @@
"type": "string"
}
}
},
"Id": {
"type": "object",
"required": [
"id"
],
"properties": {
"id": {
"type": "string"
}
}
}
}
}
Expand Up @@ -7,13 +7,13 @@
}, {
"scenario" : "disable request validation",
"stepsSummary" : {
"numberOfFailed" : 2
"numberOfSuccessful" : 3
Copy link
Member

Choose a reason for hiding this comment

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

do we still need a failed case?

}
}, {
"scenario" : "disable response validation",
"stepsSummary" : {
"numberOfSuccessful" : 3
}
} ],
"exitCode" : 1
}
"exitCode" : 0
}
Expand Up @@ -23,6 +23,7 @@ import com.twosigma.webtau.http.datanode.DataNode
import com.twosigma.webtau.http.datanode.GroovyDataNode
import com.twosigma.webtau.http.testserver.TestServerResponse
import com.twosigma.webtau.http.validation.HttpResponseValidator
import com.twosigma.webtau.http.validation.HttpValidationHandlers
import com.twosigma.webtau.utils.ResourceUtils
import com.twosigma.webtau.utils.UrlUtils
import org.junit.*
Expand Down Expand Up @@ -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") }
Copy link
Member

Choose a reason for hiding this comment

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

wonder if we should use the withAdditional... pattern we use for some other handler like classes

try {
code {
http.get("/notfound") {}
} should throwException(AssertionError, ~/(?s)header.statusCode.*404.*200/)
Copy link
Member

Choose a reason for hiding this comment

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

order is not clear based on the assertion

} finally {
HttpValidationHandlers.reset()
}
}

@Test
void "explicit status code mismatch reported before additional validators"() {
HttpValidationHandlers.register { result -> throw new AssertionError((Object)"schema validation error") }
try {
code {
http.get("/notfound") {
statusCode.should == 200
}
} should throwException(AssertionError, ~/(?s)header.statusCode.*404.*200/)
} finally {
HttpValidationHandlers.reset()
}
}

@Test
void "status code mismatch reported before additional validators and failing body assertions"() {
HttpValidationHandlers.register { result -> throw new AssertionError((Object)"schema validation error") }
try {
code {
http.get("/notfound") {
id.should == 'foo'
}
} should throwException(AssertionError, ~/(?s)header.statusCode.*404.*200/)
} finally {
HttpValidationHandlers.reset()
}
}

@Test
void "assertion and additional validation errors"() {
Copy link
Member

Choose a reason for hiding this comment

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

style of the test name mismatches the other ones.

HttpValidationHandlers.register { result -> throw new AssertionError((Object)"schema validation error") }
try {
code {
http.get("/notfound") {
statusCode.should == 404
id.should == 'foo'
}
} should throwException(AssertionError, ~/expected: "foo"/)
Copy link
Member

Choose a reason for hiding this comment

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

not clear by this check if the schema validation error is not part of the exception.

} finally {
HttpValidationHandlers.reset()
}
}

@Test
void "additional validator errors reported if status code is correct"() {
HttpValidationHandlers.register { result -> throw new AssertionError((Object)"schema validation error") }
Copy link
Member

Choose a reason for hiding this comment

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

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') {...}

try {
code {
http.get("/notfound") {
statusCode.should == 404
}
} should throwException(AssertionError, ~/schema validation error/)
} finally {
HttpValidationHandlers.reset()
}
}

@Override
String fullUrl(String url) {
if (UrlUtils.isFull(url)) {
Expand Down
4 changes: 2 additions & 2 deletions webtau-http/src/main/java/com/twosigma/webtau/http/Http.java
Expand Up @@ -436,8 +436,6 @@ private <R> R validateAndRecord(HttpValidationResult validationResult,
validationResult.setResponseHeaderNode(header);
validationResult.setResponseBodyNode(body);

HttpValidationHandlers.validate(validationResult);

ExpectationHandler recordAndThrowHandler = (valueMatcher, actualPath, actualValue, message) -> {
validationResult.addMismatch(message);
return ExpectationHandler.Flow.PassToNext;
Expand All @@ -458,6 +456,8 @@ private <R> R validateAndRecord(HttpValidationResult validationResult,
return null;
});

HttpValidationHandlers.validate(validationResult);

return extracted;
} catch (Throwable e) {
ExpectationHandlers.withAdditionalHandler((valueMatcher, actualPath, actualValue, message) -> {
Expand Down
Expand Up @@ -23,6 +23,15 @@
public class HttpValidationHandlers {
private static final List<HttpValidationHandler> configurations = ServiceLoaderUtils.load(HttpValidationHandler.class);

public static void register(HttpValidationHandler handler) {
Copy link
Member

Choose a reason for hiding this comment

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

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() {
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer to maintain as fewer resets like methods as possible.

configurations.clear();
configurations.addAll(ServiceLoaderUtils.load(HttpValidationHandler.class));
}

public static void validate(HttpValidationResult validationResult) {
configurations.forEach(c -> c.validate(validationResult));
}
Expand Down