OBPIH-5936 initial rest assured API tests#4600
OBPIH-5936 initial rest assured API tests#4600ewaterman merged 7 commits intofeature/upgrade-to-grails-3.3.10from
Conversation
ewaterman
requested review from
alannadolny,
awalkowiak,
drodzewicz,
jmiranda and
kchelstowski
|
EDIT: I took these words to heart and reworked this PR to use @Integration instead. Now our tests live under /src/integration-test and can take advantage of all the things I mention here. Hurray! Honestly, this whole process has made me reconsider if API testing is worth the hassle for us. I spoke with some of the OpenMRS team and they don't do API testing for similar reasons to the struggles we've run into here. Spring/Grails gives us the @Integration method of integration testing which props up the whole app context in memory and essentially allows us to API test (via mock request clients) without actually starting the server. We can even prop up a containerized database (via testcontainer) so that we're actually testing against a real db. It also allows us to swap in different databases (if we ever want to run the tests against both mysql and mariadb). The obvious drawback to this integration testing is we're not testing against a real, running server, but if we have good e2e tests in place, we don't really need to. I think next steps for us is to try and get integration tests running again and if that works for us, maybe that's good enough. But if they don't work, API testing is likely our best option. |
jmiranda
changed the title
jmiranda
changed the title
ewaterman
added
the
warn: do not merge
There was a problem hiding this comment.
I had to disable sentry the hard way to get these tests running. We don't want sentry doing any reporting when we're running tests but I didn't have the time initially to investigate the best way to conditionally disable sentry based on which profile is running (test). I'm sure there's an easy way, I just haven't found it yet.
If anyone has thoughts, I'm all ears.
There was a problem hiding this comment.
hah I'm just gonna steal Justin's solution from #4601
There was a problem hiding this comment.
Yeah I was going to say, we just need to add logback-test.xml or whatever. But that was a quick dirty hack to avoid an error message, we should do more investigation to make sure it's the right approach.
ewaterman
changed the title
ewaterman
removed
the
warn: do not merge
| JSONObject buildAuthRequestBody(String username, String password, String location) { | ||
| return new JSONObject() | ||
| .put("username", username) | ||
| .put("password", password) | ||
| .put("location", location) | ||
| } |
There was a problem hiding this comment.
This looks like it can be some kind of a generic function that can be used in other tests to convert Map to JsonObject
some kind of a JSONObject bodyBuilder(Map data) { ... } wdyt? 🤔
There was a problem hiding this comment.
@drodzewicz JSONObject already has a constructor that takes a map as an argument. For request bodies that are relatively small, this version might be ok, but for bigger I think it might be better to build from a map as you suggested. But I am not sure if this is something that we have to tackle now.
There was a problem hiding this comment.
A JSONObject is already essentially a map from what I can see so I'm not sure what advantages we'd get working with maps then converting them to JSONObjects.
If we were using request DTOs (data transfer objects) then we could use those directly here to build the request object, but since the controller just doing this:
def category = Category.get(params.id)
if (!category) {
category = new Category(request.JSON)
} else {
category.properties = params
}
this is the best we can do for now. (And I didn't want to use Category itself because IMO that's peeking too much into the method itself. As much as possible, these tests should be treating the backend as a black box.)
awalkowiak
left a comment
awalkowiak
left a comment
There was a problem hiding this comment.
Overall I think it LGTM (with minor concern/discussion around building request json objects).
Hhowever when I tried to pull this locally and play around with it when I ran a simple grails test-app -integration it resulted with failed tests. I tried to look around for any preconditions or anything like that, but since this one is documented in three different places, I could not find the exact answer to my simple question "How can I run it locally?" (or I am blind). I think it would be good to extend the README.md here https://github.com/openboxes/openboxes?tab=readme-ov-file#14-grails-tests and split the backend testing part into two separate cases - unit and integration, and document what needs to be done to run each (at the current state).
| import io.restassured.response.Response | ||
| import io.restassured.specification.RequestSpecification | ||
| import org.apache.http.HttpStatus | ||
| import org.json.JSONObject |
There was a problem hiding this comment.
So far we've been using the one from org.grails.web.json.JSONObject
| JSONObject buildAuthRequestBody(String username, String password, String location) { | ||
| return new JSONObject() | ||
| .put("username", username) | ||
| .put("password", password) | ||
| .put("location", location) | ||
| } |
There was a problem hiding this comment.
@drodzewicz JSONObject already has a constructor that takes a map as an argument. For request bodies that are relatively small, this version might be ok, but for bigger I think it might be better to build from a map as you suggested. But I am not sure if this is something that we have to tackle now.
@awalkowiak Yeah tests aren't in a running state yet because I still have to go clean up some of the old broken ones but if you run these ones individually they should pass. I wanted to wait until this got merged in before updating all the docs in case we decided to change anything up but yes 100% we should have two separate test flows. My ultimate goal is to have both unit and integration tests running against all PRs but that's going to come from a follow up task I think since this one is big enough already. Edit: Actually, I remember now that I did clean up some existing tests and so running
|
4 participants
This is a functional prototype of how we could do Integration/API testing. This is the result of some preliminary investigation done here: https://pihemr.atlassian.net/wiki/spaces/OB/pages/3041001477/DISCOVERY+OBPIH-6364+Investigate+Integration+API+Testing+Approaches
I used the following PR as a reference: #4360
The JIRA ticket: https://pihemr.atlassian.net/browse/OBPIH-5936
I broke the API tests out into their own gradle project (instead of putting them in a api-test package in the app itself) because api tests should be able to run in an independent process from the app, and should treat the app as a black box.^ I decided against this. Read my comment below for a quick summary or the discovery document for an in depth discussion.
My primary approach here was to have tests:
This way we can run any single test in isolation or the whole suite together without any tests affecting each other.
With this approach, the general structure of tests is:
Future Considerations: