[sip-comm-dev] jUnit tests for RSS - _very_ preliminary version

Benoit Pradelle ze_real_neo at yahoo.fr
Mon Sep 17 20:56:49 CEST 2007


Hi Mihai,

I've reviewed and commited your Rss tests proposal and it's a really 
good job: your code is very clean, structurated, consistent and efficient.
In three words, very nice work :)

As every big patchs, there are some little remarks and questions (which 
absolutely don't mean anything on the overall quality of your code)

- As minor esthetic changes I added the licence header on some of the 
files, corrected some tabulation miss, correct characters inside the 
dead zone (aka after 80 characters on a line), corrected some javadoc 
problems

- In RssProtocolProviderServiceLick.start(...), why have you written 
"logger.setLevelAll()" ? I don't know exactly what does this method but 
it seems overwriting the default logger settings, is it really needed ?

- I corrected many "RSS" with a new entry in the ProtocolNames interface

- In the TestingServer constructor, your bind the server with 
"127.0.0.7" however the default IPv4 loopback address is 127.0.0.1 and 
this static string is a little bit mind-distrubing for me as I'm not 
really sure that this works with IPv6-only net interfaces so I replaced 
this string with null which is explicitly associated with the loopback 
interface in the javadoc.

- I changed the value of your constants INVALID, VALID, VALID_NEW and so 
on... because first the hexadecimal notation is here not needed and may 
appear terrible for some people :) and more seriously because you 
previously gave a power of two value for these constants. I perhaps have 
a not normal brain but when I see these values I immediately think at 
some binary or-able (constants that you can combine with the binary or 
operator) flags which is not the case here so I simply gave them new 
values (0, 1, 2, 3)

- don't you think that using the 8080 port as a default value may cause 
some problem to the users who are already using this port for doing 
something else ? Isn't it a perfect choice for an account parameter in 
the account.properties file ?

- in TestingServer.start you got this:
        //only allow one "instance" of the server to be running at a 
given time
        if (serverActive)
            return;
       
        //create new thread and launch
        runner = new TestingServerThread(this, server);
        serverActive = true;
It is a good idea to avoid having two server launched in the same time 
but your code isn't thread safe here: it is possible to create two 
servers if the method is called by two concurrent threads. Did you 
envisage this case ? Is it grave ? This can be corrected easily with a 
little "synchronize" :)

- and last, in TestingServerThread.run you got:
        if (launcher.isActive())
before doing anything. Why did you write this test ? Do you know that if 
the launcher didn't end its initialization phase, the server may never 
start ? Moreover, is this test really useful ?

And that's all ;)

As you said in your mail, for the moment the tests are limited and the 
rss functionalities aren't really tested but the server is ready and 
it's a very good point: it should now not be so difficult to implement 
some more Rss specific tests :)

Most of these remarks are very minors one, so as I previously said, very 
good job Mihai and thanks for your contribution !

Cheers,
Ben

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe at sip-communicator.dev.java.net
For additional commands, e-mail: dev-help at sip-communicator.dev.java.net





More information about the dev mailing list