-1

I am on the java tomcat stack and creating a new filter. https://javaee.github.io/javaee-spec/javadocs/javax/servlet/http/HttpFilter.html I am interested in unit testing it because I want to have 100% branch coverage.

This filter wraps the response object. We override the default behavior of the response such that whenever we call response.addCookie(cookie), we append the string "happy" to the cookie name:

HappyCookieFilter implements Filter {
HappyCookieResponseWrapper happyCookieResponseWrapper;
...

public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) {
    chain.doFilter(req, happyCookieResponseWrapper.wrap(res));
}

...
}
  1. Assuming the HappyCookieResponseWrapper is already unit tested, what would be the benefit of testing doFilter method?
  2. How would I test the HappyCookieFilter.doFilter and what should I assert?
Eric Cherin
  • 233
  • 2
  • 10
  • What is `response`? If it is supposed to be the same as `res`, then why would a happyCookie**Response**Wrapper.wrap(**response**) call be returning a `ServletRequest` object? – Andreas Jan 17 '20 at 09:11
  • I would definitely unity test the `doFilter` method. The test would guarantee that the wrap function on `HappyCookieResponseWrapper` was called. I have seen too many errors happing when some component or class is perfectly programmed and tested and then forgotten to use or used in wrong fashion. That is a behaviour unit test. – Michal Jan 17 '20 at 09:11

3 Answers3

1

"What would be the benefit of testing doFilter method?"

None!

Quoting answer to the question "Should unit tests be written for getter and setters?":

Unit tests are there to test the behaviour of your code, ...

There is really no behavior to be tested in that Filter code. The behavior to be tested is in the HappyCookieResponseWrapper class, and you're already testing that. Repeating that test would just be a waste of time.

"I want to have 100% branch coverage"

Quoting a different part of the same answer above:

@Will said you should aim for 100% code coverage, but in my opinion that's a dangerous distraction. You can write unit tests that have 100% coverage, and yet test absolutely nothing.

Community
  • 1
  • 1
Andreas
  • 154,647
  • 11
  • 152
  • 247
  • My colleague wants to have 100% coverage. Don't we want to assert that the chain.doFilter was called? Isn't that a behavior? What benefit does it provide (theoretically), no matter how small? – Eric Cherin Jan 17 '20 at 09:04
  • @EricCherin *Unit* testing the filter doesn't actually test that your full web application correctly installed the filter. That would be a *system* test, if anything, not a unit test. – Andreas Jan 17 '20 at 09:06
  • The unit test would be: ... HappyCookieFilter.doFilter(... verify(chain).doFilter(); – Eric Cherin Jan 17 '20 at 09:08
  • 1
    @EricCherin That is as useless a test as the testing of a plain getter method, in my opinion. – Andreas Jan 17 '20 at 09:14
  • I agree that you should never aim to have 100% test coverage. You try to test all logic that you write yourself. In this case you would test a method on an object that exists in a library that isn't even yours. This would be the same as testing the java.lang.String class. – Wesley De Keirsmaeker Jan 17 '20 at 09:14
0

If the doFilter() method has 'interesting' logic like special filtering or routing rules, testing them whould be the purpose of testing this void method. Generally speaking, if a void method has logic to create side effecs (apart from just calling chain.doFilter() in your case) then the purpose of testing said void method would be to test that logic. On the other hand this logic could and should live in a non-void method (maybe in another class) that could be tested on it's own.

Having 100% coverage (line or branch) is nothing to aim for (especially if you don't make sure that the code isn't only covered but also asserted).

Nicktar
  • 5,548
  • 1
  • 28
  • 43
0

It is a perfectly valid class and method to unit test.

Test in chain.doFilter whether (the intended behavior of happyCookieResponseWrapper.wrap(response) was passed as request. No technicalities. Maybe that a cookie was set in the request or so. The business logic and functioning of happyCookieResponseWrapper need not be repeated. There might be an elementary overlap (cookie was set), but that happens at two different levels.

By the way I am a bit surprised seeing a response wrapper for a request, but as it compiles it'll be alright.

It is a very narrow/trivial check. But assume this code at some time in the future is redesigned to use an other wrapper class, that does not fulfill the correct behavior. The regression error will then be found fast, as opposed to hearing from a fuzzily described error and processing a ticket.

Joop Eggen
  • 107,315
  • 7
  • 83
  • 138