[rfc][icedtea-web] refactored logging

[Omair Majid]

Hi Jiri,

On 09/26/2013 03:25 AM, Jiri Vanek wrote:
> On 09/25/2013 07:09 PM, Omair Majid wrote:
>> On 09/25/2013 12:52 PM, Jiri Vanek wrote:
>>> good tests :) Especially I was happy teh paralel logger test passed
>>> always :)
>>
>> I thought the code review policy was that all code must be reviewed? Are
>> unit tests exempt from this?
>>
> 
> Definitely not. Sorry for rush.

One of the advantages of writing unit tests is that it forces you to
reduce coupling. I was hoping that the design of OutputController will
be improved if you wrote tests first.

> Do you mind to eyeball?

Lots of comments below. You don't have to address them all, but please
use this opportunity to learn more about unit tests.

> http://icedtea.classpath.org/hg/icedtea-web/rev/a817bb6d12a6#l117.1
> http://icedtea.classpath.org/hg/icedtea-web/rev/a817bb6d12a6#l116.1
> http://icedtea.classpath.org/hg/icedtea-web/rev/a817bb6d12a6#l115.1

I think a lot of these tests are more complex than necessary and are
doing too much. There are lots of resources that cover what a good unit
test consists of [1][2] and what makes a bad unit test [3][4].

Generally, unit tests should test one unit (not a system) at a time.
They should not interact with the resources like file system (maybe the
unit test for a class solely logging to a file is an exception), or
threading.

I see a few specific cases that I think can be made much better.

> @Test
> public void isAppendingLoggerLoggingOnNotExisitngFile() throws Exception {
>     int i = 0;
>     FileLog l = new FileLog(loggingTargets[i].getAbsolutePath(), true);
>     l.log(line1);
>     String s = StreamUtils.readStreamAsString(new FileInputStream(loggingTargets[i]), true);
>     Assert.assertTrue(r1.evaluate(s));
> }

This might be much easier to read if it were written as:

@Test
public void testThatLoggerCanAppendToNonExistingFile() {
   String LINE = "whatever this is";
   File target = new File("file name");
   deleteFileIfExists(target);
   FileLog log = new FileLog(target, /* append = */ true);
   log.log(LINE);
   String s = StreamUtils.readStreamAsString(
     new FileInputStream(target), true);
   assertEquals(LINE, s);
}

What i is doing is not obvious in the first case, but more obvious in
the second. Is this value of i even reused in any other test?

I am really surprised by the RulesFolowingClosingListener. If you need
matchers, why not use the standard junit/hamcrest matches or even
introduce new ones conforming to the standard interface? Personally, I
prefer exact strings, though. That way, if you accidentally add more
contents to a string, the unit tests will still detect them.

I am rather sad to see OutputControllerTest. Pretty much all the tests
are trying to do too much.

The tests are showing that LogConfig is too tightly coupled with
OutputController (which is not obvious from the class interface - it's
through a global singleton). The first question that comes to me is why
"log" configuration related to OutputController. LogConfig should be
renamed to something more accurate. There should also be a
OutputControllerFactory (or similar) class that uses the config to
create a OutputController.

An OutputController should be injectable with as many logger
implementations (syslog, file, std stream) so the behaviour of each
logger implementation can be tested separately from the behaviour of
OutputController.

Some of the logic should be part of a separate test. For example,
isLoggingStdStreams needs to be split into a test for
OutputControllerTest and a test for PrintStreamLogger. The test in
OutputControllerTest should inject a fake PrintStreamLogger and just
check that it's called correctly. The test for PrintStreamLogger should
check that the actual string matches.

The parallel thread test is (to me) really asking for the threading
implementation of OutputController to be split out. If you can split out
the queue so it has no interaction with thread-related APIs, you can
test the queue implementation without any threading at all.

Please also add tests for other new functionality such as DebugPanel.

Thanks,
Omair

[1] http://stackoverflow.com/questions/61400/what-makes-a-good-unit-test
[2] http://esj.com/Articles/2012/09/24/Better-Unit-Testing.aspx
[3]
http://stackoverflow.com/questions/3913912/what-are-bad-attributes-in-unit-tests
[4] http://howtodoinjava.com/2013/06/10/8-signs-of-bad-unit-test-cases/

-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681