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

Make JsonSchemaAssertions check for JSON content-type #65

Conversation

mariacamenzuli
Copy link
Member

Added assertions on the response content type in JsonSchemaAssertions.

This fixes #59

@mariacamenzuli
Copy link
Member Author

This change was simply copying the assertions on the content type from JsonResponseAssertions over to JsonSchemaAssertions. Can we come up with some kind of abstraction for the capabilities of the assertion classes to avoid this duplication? @FrelliBB @KPull

@KPull
Copy link
Member

KPull commented Dec 31, 2016

Can we come up with some kind of abstraction for the capabilities of the assertion classes to avoid this duplication?

Yes. I can forsee that an Assertions object would typically want to assert the status code and content-type for example. We can abstract this into a CommonAssertions object (just like we have CommonRequestAttributes), and have the applicable assertions inherit from CommonAssertions by composition.

Copy link
Member

@KPull KPull left a comment

Choose a reason for hiding this comment

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

Nice pull request 👍. Do you want to make the CommonAssertions abstraction yourself in this pull request or not?

@@ -55,6 +60,12 @@ public void execute(int statusCode,
}
}

public JsonSchemaAssertions overrideContentType(ContentType contentType) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add JavaDocs to this public method, please? I think you can add the same JavaDocs as JsonResponseAsserions.

* @param contentType
* @return A model response object.
*/
static ModelResponse<String> prepare(String jsonContent, String contentType) {
Copy link
Member

Choose a reason for hiding this comment

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

This method appears to be a generalisation of the method above it. I would rewrite (and rename) the method above this one to use the new method you wrote.

Added javadoc and refactored helper method to use a more general method.
@mariacamenzuli
Copy link
Member Author

I think it's best if we refactor the structure of the assertions in a separate pull request. I can open an issue.

@mariacamenzuli
Copy link
Member Author

I've applied the other changes requested.

@mariacamenzuli
Copy link
Member Author

Opened #66 for the assertion abstraction task.

Copy link
Member

@KPull KPull left a comment

Choose a reason for hiding this comment

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

Good work! Merging! :shipit:

@KPull KPull merged commit 8a8b5ac into bastion-dev:develop Jan 2, 2017
@KPull KPull added this to the 0.5-BETA milestone Jan 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants