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
//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:
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 !
To unsubscribe, e-mail: firstname.lastname@example.org
For additional commands, e-mail: email@example.com