Get rid of our REST testing classes in favor of REST Assured #429
Conversation
I mistakenly thought that the other callback was not executed on test failure but it is indeed executed and we want all the other tests to be able to run.
|
Hmmm, there is a weird test failure when running the native image tests, looking. |
|
OK, CI is happy now. |
| @@ -52,8 +52,7 @@ public static JavaArchive deploy() { | |||
|
|
|||
| @Test | |||
| public void testHealthServlet() { | |||
| URLResponse rep = URLTester.relative("health").invokeURL(); | |||
| Assert.assertEquals(503, rep.statusCode()); | |||
| RestAssured.when().get("/health").then().statusCode(503); | |||
There was a problem hiding this comment.
You can directly use: get(...), but it's a question of flavors.
| // the health check does not set a content type so we need to force the parser | ||
| try { | ||
| RestAssured.defaultParser = Parser.JSON; | ||
| RestAssured.when().get("/health").then() |
There was a problem hiding this comment.
As you use static import for hamcrest, why not using the same for rest assured and so just do:
get("/health")...
There was a problem hiding this comment.
It was more so that people know what they are using.
People are used to hamcrest assertions being static but I didn't want a magic get(...) that people don't know where it's coming from.
| Assert.assertEquals("basic", check.getString("name")); | ||
| // the health check does not set a content type so we need to force the parser | ||
| try { | ||
| RestAssured.defaultParser = Parser.JSON; |
There was a problem hiding this comment.
Should it be in a beforeClass method?
There was a problem hiding this comment.
Well, it all depends on what your service returns so I prefer to do it on a per call basis.
I'm not very familiar with this health thing so I don't know if it can also produce XML. But I decided go per call to be on the safe side.
I can change it if you prefer though.
There was a problem hiding this comment.
I believe it can only return JSON (at least in the initial version of the spec).
@kenfinnigan can you confirm?
There was a problem hiding this comment.
BTW, it would help if it included a proper content type. We could then remove these calls.
There was a problem hiding this comment.
@cescoffier maybe we could merge and adjust later if needed? I'm a bit worried that we could have annoying conflicts, considering it touches a lot of places.
There was a problem hiding this comment.
I believe the spec only defines JSON return type, yes
| "checks.state", contains("UP"), | ||
| "checks.name", contains("basic")); | ||
| } finally { | ||
| RestAssured.reset(); |
There was a problem hiding this comment.
Should it be in an afterClass method?
|
Yes,
I believe it should be fixed in smallrye actually.
…Sent from my iPhone
On 4 Jan 2019, at 17:33, Guillaume Smet ***@***.***> wrote:
@gsmet commented on this pull request.
In extensions/health/deployment/src/test/java/org/jboss/shamrock/health/test/HealthUnitTest.java:
> @@ -26,15 +26,16 @@ public static JavaArchive deploy() {
@test
public void testHealth() {
- JsonReader parser = URLTester.relative("health").invokeURL().asJsonReader();
- JsonObject obj = parser.readObject();
- System.out.println(obj);
- Assert.assertEquals("UP", obj.getString("outcome"));
- JsonArray list = obj.getJsonArray("checks");
- Assert.assertEquals(1, list.size());
- JsonObject check = list.getJsonObject(0);
- Assert.assertEquals("UP", check.getString("state"));
- Assert.assertEquals("basic", check.getString("name"));
+ // the health check does not set a content type so we need to force the parser
+ try {
+ RestAssured.defaultParser = Parser.JSON;
@cescoffier maybe we could merge and adjust later if needed? I'm a bit worried that we could have annoying conflicts, considering it touches a lot of places.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@cescoffier agreed it should be fixed in smallrye, just not sure which project is the culprit though. @kenfinnigan can probably help with that. In the mean time, I think what I did is acceptable and we can get rid of it as soon as it's fixed upstream and we have updated Shamrock. I don't think we will add new tests to these classes anyway. |
|
Can you clarify what should be "fixed" in SmallRye? Can then take a look |
|
@kenfinnigan currently, the end point does not set the content-type of the response (e.g. I'm talking about REST Assured but it could be an issue for a lot of usages: it is considered a good practice to define a proper content-type for your response. I took a quick look at the SmallRye health project but it seems to write to an |
|
Gotcha, we can certainly look to fix that for SmallRye Health |
|
@kenfinnigan so I'll merge this for now, and when SmallRye is fixed, please ping me so I can remove the workaround. |
cescoffier
added
kind/enhancement
REST assured was used in a few places but not consistently.
In passing, fix an issue in the test framework changes I made a few weeks ago.