Campoie / rest-assured

Automatically exported from code.google.com/p/rest-assured
0 stars 0 forks source link

Add support for logging if test fails #212

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Now you can do e.g. log().ifError() but that only looks at status code. So for 
example if you expect 400 and log().ifError() and receive a 200 then nothing 
will be printed.

Original issue reported on code.google.com by johan.ha...@gmail.com on 17 Dec 2012 at 11:35

GoogleCodeExporter commented 9 years ago
To me it looks like it would suffice to add something along the lines of

logWith(new ResponseLoggingFilter(getPrintStream(), 
isNot(responseSpecification.getStatusCode())))

to ResponseLogSpecificationImpl. Unfortunately I seem to be unable to build 
rest-assured here at the moment to verify that. 
Thoughts?

Original comment by wolfgang...@dfki.de on 23 Jan 2014 at 3:40

GoogleCodeExporter commented 9 years ago
That's a really nice solution! Too bad I just released a new version otherwise 
I could have included this. Thanks for the tip!

Original comment by johan.ha...@gmail.com on 23 Jan 2014 at 6:15

GoogleCodeExporter commented 9 years ago
Would love this to be supported in all given implementations

Original comment by jmagnu...@gmail.com on 1 Feb 2014 at 12:10

GoogleCodeExporter commented 9 years ago
To clarify, I run into this problem when I try to use given( 
RequestSpecification, ResponseSpecification )

I use the ResponseSpecBuilder to build out my ResponseSpecification with 
ResponseSpecification = ResponseSpecBuilder.build().  The problem seems to be 
that ResponseSpecBuilder doesn't support log(LogDetail.ALL) like 
RequestSpecBuilder does.

At any rate... to work around this and log the service response I do this 
beautiful hackish typecast:

String json = 
   ((TestSpecificationImpl)given( reqSpecification, resSpecification )).getResponseSpecification().
      log().all()   or     log().ifError()    or the proposed logWith()   .
        get("your_api").asString();

You still get the AssertionErrors as before, but you at least get the response 
body logged out.  If you really want to hack around the long stack trace for 
clean test output (which I don't recommend):

try {
  RestAssured given() . get() / put() / post() / head() / etc
} catch ( Throwable t ) {
  print nicely
  use a java or junit assert?  throw t?
}

Original comment by jmagnu...@gmail.com on 1 Feb 2014 at 1:04

GoogleCodeExporter commented 9 years ago
@jmagnus: That's a separate issue. Please report it as a new issue that we 
should add logging support for ResponseSpecBuilder. I remember that I tried to 
do that once but for some forgotten reason I never pulled through.

Original comment by johan.ha...@gmail.com on 9 Feb 2014 at 3:10

GoogleCodeExporter commented 9 years ago
@ wolfgang: Unfortunately I don't think your proposal will work since there can 
be other errors that cause test failure (for example a json path may not be as 
expected). 

Original comment by johan.ha...@gmail.com on 25 Mar 2014 at 2:35

GoogleCodeExporter commented 9 years ago
Hm, true, my proposal does not cover all possible test failures.

I still think it would be valuable to have something along the lines of 
log().ifUnexpectedStatus()
It may not be true to the letter of the subject of this bug, but at least in 
the spirit of the original report, which specifically mentions printing on an 
unexpected 200.

Although, if you would prefer to handle the general case in this bug and my 
request separately, I'll gladly file another report. 

Original comment by wolfgang...@dfki.de on 25 Mar 2014 at 2:52

GoogleCodeExporter commented 9 years ago
I'm thinking that perhaps this should be enabled by default? When a test fails 
both the request AND response is logged to the console (unless configured 
otherwise). What do you think about this?

Original comment by johan.ha...@gmail.com on 25 Mar 2014 at 2:56

GoogleCodeExporter commented 9 years ago
In my experience, most of the time Hamcrest's descriptions are fine. Only in 
the minority of testcases I've writen did I want to see the complete exchange 
on error.
Although you're right, if that level of detail is needed, I usually want to see 
both request and response.

Of course, since the general case (logging on any failure) subsumes logging on 
unexpected return code, I'm fine with either being implemented.

Seeing as you want to make HTTP exchange logging configurable, I for one am 
fine with your proposal. Maybe one of the other three watchers of this bug want 
to chime in?

Original comment by wolfgang...@dfki.de on 25 Mar 2014 at 3:15

GoogleCodeExporter commented 9 years ago
Perhaps it should be turned off by default anyway and we'll wait and see what 
people think.

I'm still a bit uncertain of how to best implement this feature from an API 
perspective. Currently you do something like:

given().log().all() to do request logging (by using filters) and 
..then().log().all() for response logging. It seem rather strange to me to have 
a given().log().ifTestFails() that both enables request and response logging at 
the same time. It's not consistent with the way logging works right now (either 
with request or response logging). Perhaps it's better to keep it like it is 
today and allow you do explicitly specify "ifTestFails()" for both the request 
and response. There can be another way to enable it for both at the same time 
by using the LogConfig or something: 

given().config(newConfig().logConfig(logConfig().logEverythingIfTestFails())). 
.. 

We could also enable a static shortcut for this: 

RestAssured.logEverythingIfTestFails();

and maybe:

given().logEverythingIfTestFails() (although this is perhaps a little bit 
inconsistent?)

What do you think?

Original comment by johan.ha...@gmail.com on 26 Mar 2014 at 7:46

GoogleCodeExporter commented 9 years ago
I agree. In the interest of consistency and minimizing surprise for the user, I 
would also prefer to have to specify "ifTestFails()" for both request and 
response, just as it is with "all()" currently.

LogConfig is, as far as I can see, an appropriate place for such a global 
switch. I personally don't have strong feelings about the shortcuts one way or 
another.

Original comment by wolfgang...@dfki.de on 26 Mar 2014 at 12:59

GoogleCodeExporter commented 9 years ago
Thanks for your comments. I'll see what I can do when I find some time :)

Original comment by johan.ha...@gmail.com on 26 Mar 2014 at 1:14

GoogleCodeExporter commented 9 years ago

Original comment by johan.ha...@gmail.com on 27 Mar 2014 at 2:44

GoogleCodeExporter commented 9 years ago
I've implemented support for this now. A new snapshot has been released, depend 
on version 2.3.1-SNAPSHOT after having added the following repo:

<repositories>
        <repository>
            <id>sonatype</id>
            <url>https://oss.sonatype.org/content/repositories/snapshots/</url>
            <snapshots />
        </repository>
</repositories>

You can now do like this:

given().log().ifValidationFails() .. 

for request logging and:

.. then().log().ifValidationFails(). ..

for response logging. 

To enable both at the same time you can use the LogConfig or the following 
shortcut:

RestAssured.enableLoggingOfRequestAndResponseIfValidationFails();

This has also been implemented for RestAssuredMockMvc. 

Please try it out and tell me what you think, I'd like to make a new release 
soon.

Original comment by johan.ha...@gmail.com on 28 Mar 2014 at 10:54

GoogleCodeExporter commented 9 years ago
Just tried it, looks good to me. Thanks for adding this feature!

Original comment by wolfgang...@dfki.de on 31 Mar 2014 at 12:17