Hi Petr,
On Wed, Jul 1, 2015 at 10:19 PM, Petr Shypila <[email protected]> wrote:
> ... I also wrote it in another way which
> becomes easier to understand[2]. Here I just mock objects on a first level
> and then check that they were called with correct parameters and I also
> check that these parameters have correct values...
This is much better and fairly readable, I agree.
The code at [2] however contains an lot of duplication, you should
refactor the common things to make it more readable and manageable.
I'm also not convinced about assertions such as
assertNotEquals(405, res.getStatus());
assertNotEquals("GET, HEAD, POST, PUT, DELETE, OPTIONS",
res.getHeader("Allow"));
It's not obvious at all what those tests, and they leave a whole range
of possible invalid values IMO - in general, tests should verify a
single or a small range of expected values. Here I suspect that your
tests make too many assumptions about the internal code, which might
make the tests brittle.
So in summary, this is going in the right direction but needs work!
-Bertrand
> [2]
> https://github.com/PetrShypila/sling-builder/blob/engine-module-simple/bundles/engine/src/test/java/org/apache/sling/engine/impl/SlingMainServletTest.java
>
> -Petr
>
> 2015-06-29 14:34 GMT+02:00 Petr Shypila <[email protected]>:
>
>> Yes, thank you. I'll do it in this way.
>>
>> Best Regards,
>> Petr Shypila
>>
>> > On 29 Jun 2015, at 14:23, Bertrand Delacretaz <[email protected]>
>> wrote:
>> >
>> > Hi Petr,
>> >
>> >> On Mon, Jun 29, 2015 at 10:40 AM, Petr Shypila <[email protected]>
>> wrote:
>> >> ...could you please take a look on my
>> >> first test for SlingMainServlet[1]...
>> >
>> > I think it's a good start, but I don't think it's sufficient to just
>> > verify that the SlingRequestProcessor is called - you should verify
>> > that it is called with the appropriate values, and that its actions
>> > are taken into account. Best might be to mock the
>> > SlingRequestProcessor (or maybe it's easier to instantiate a
>> > SlingRequestProcessorImpl with a few mocks) and test both classes in
>> > combination, which is how they are used in practice.
>> >
>> > This diverges a bit from pure unit tests which should arguably test a
>> > single class, but we don't really care here IMO. Testing in a more
>> > realistic situation (so SlingMainServlet combined with
>> > SlingRequestProcessorImpl) will probably make your tests look more
>> > natural, which is good IMO.
>> >
>> > Does this work for you?
>> > -Bertrand
>> >
>> >
>> >> [1]
>> >>
>> https://github.com/PetrShypila/sling-builder/blob/d4d04af973876aced8dd042ee9b3880ad4e00449/bundles/engine/src/test/java/org/apache/sling/engine/impl/SlingMainServletTest.java
>>
