I eventually went for your option 2. See my comments inline below...
I've tested the first set of six changes and am pleased with the result. However, I haven't yet committed them - I want to confirm you are happy with the approach before I go ahead.
If you are happy, I'll make the second set (of two) changes and commit them separately.
Although I've registered to get my ICQ identity for testing, I still
haven't created my own accounts.properties file in the lib directory
(don't nag me!). The wiki mentions this as a known problem, but I do
not get the symptoms described. I see the following...
java.lang.IllegalArgumentException: value of format cannot be null at
net.kano.joscar.DefensiveTools.checkNull(Unknown Source) at
net.kano.joustsim.Sreenname.<init>(Unknown Source) at
The accounts.properties file is only relevant to the automated tests. It
contains information for accounts that the test suites use in order to
send and receive test messages and also retrieve and register various
I propose to detect this crucial and common problem as early as
possible and trigger a runtime failure with an unchecked Exception
and a more meaningful message, e.g.
java.lang.IllegalStateException: account.properties file not found in
lib directory (please see wiki for advice on unit test setup)
Yes I agree. That and something else. The unit testing process runs in
two stages (roughly). Here's how it goes:
Test suites for the various services are grouped in separate bundles
called SLICKs. The first stage of the testing process consists in
loading these bundles and calling their start() method which would
_only_ initialize them. Note that _NO_ tests are actually run here (i.e.
no testAbcXyz() methods are called).
The last bundle that gets loaded during testing is the SlickRunner
bundle. Once it is loaded it discovers all Slick bundles that have
registered and executes the actual tests. That's when the actual testing
is being run and where the testAbcXyz() methods get called.
Throwing an IllegalStateException if the accounts.properties file is not
available would cause a stack trace dump during the first stage. Things
would remain unchanged in the second stage and the reason for the
failure would not be visible in the test results html files
(proj_root/test-reports/html). These test results would actually contain
a single error for the ICQ slick saying that no such slick was found
(this is becaulse the exception would have interrupted the slick
registration). I believe that most users would remain quite perplexed at
Yes, I could see that you were correct when simply making my original proposed change and lighting the blue touchpaper. The Oscar log showed the IllegalStateException, so if you looked at the log the message
about the missing accounts.properties file was loud and clear.
However, throwing the ISE early meant IcqProtocolProviderSlick.start() didn't have time to set up the unit tests (which would have failed anyway). It all looked quite good until I checked the html test reports. There
was a single ERROR because no tests were found to be executed.
Clearly, the quick solution wasn't good enough to avoid all the (future) questions on the mailing list!
There are two ways to go here:
1) During the first phase, when the icq slick is being loaded, we detect
that the accounts.properties file is not there. We log an error and we
bravely continue so that the icq slick and all its tests get registered.
Then we make sure that when the tests are being run they fail with the
appropriate error and wiki reference (or at least those that require
I looked at some of the existing test methods and they are fairly heavyweight. I realised I could write a little subroutine that they could assertTrue against, but that would have been liable to programming errors,
especially if someone wrote a new unit test and forgot to check the environment.
I then thought about doing an assertTrue in the setup() method and failing every test before it even starts. However, there are seven test classes, so each would need the same setup assertion.
2) During the first phase, when the icq slick is being loaded, we detect
that the accounts.properties file is not there. At this point we
completely replace the set of tests that we register for later execution
with a single test that always fails with the appropriate message and
I personally believe that 1 is somewhat more logical but I don't mind 2)
So, I decided option 2 was the best after all. My new logic is as follows...
1. IcqProtocolProviderSlick.start() now detects the lack of a properties file, so it knows there isn't any point running the normal tests.
2. We can't run with zero tests, because the resulting html report isn't specific enough.
3. I've written an new class called TestAccoutInvalidNotification. It currently has one instance method called failIcqTesterAgentMissing that simply calls the junit fail() method with a highly meaningful message.
4. IcqProtocolProviderSlick.start() sets up to run a single specific test -TestAccoutInvalidNotification.failIcqTesterAgentMissing. The meaningful failure message appears in the html report, rather than a non-specific
error message. Note: we get a jUnit test Failure reported, rather than an Error - which is the appropriate behaviour.
5. IcqProtocolProviderSlick.stop() would blow up with an NPE, because IcqSlickFixture.testerAgent is null and so its unregister() method cannot be called. I have protected the call by testing for a null value.
6. MetaContactListServiceLick.stop() blows with an NPE because MclSlickFixture.mockP1ServiceRegistration is null when the accounts.properties file is not present. I have NPE-protected each of the three
Also, I propose to make two other associated changes....
2. IcqProtocolProviderSlick.start() has existing logic to throw an Exception if the testerAgent.register() method fails. I propose to replace the Exception with an (analogous) specific failure unit test.
3. IcqProtocolProviderSlick.start() will then have three different logic paths, each with different unit test suites to be registered. I propose to refactor the start() method to make the logic simpler to follow - I'll setup the
specific unit tests in private methods, rather than inline.
On Mon, 19 Jun 2006 19:10:31 +0200, Emil Ivov wrote:
To unsubscribe, e-mail: email@example.com
For additional commands, e-mail: firstname.lastname@example.org