[sip-comm-dev] An old issue with the testing environment


#1

Hi, Emil. I've had an item on my list of things to do for about 4 months and I think I might find some time to work on it fairly soon.

I don't want to commit a change until I've had your agreement on the general idea.

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 net.java.sip.communicator.slick.protocol.icq.IcqTesterAgent.<init>(blah)
      at net.java.sip.communicator.slick.protocol.icq.IcqProtocolProviderSlick.start(blah)

I realise the user-oriented release of the sip-communicator will ensure a suitable accounts.properties file is created, but that isn't relevant to the unit tests.

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)

I don't think we need a unit test to prove the new logic works.

What do you think? I've set the breakpoints my debugger, all ready to trace the execution sequence and identify the best place to throw the new Exception.
Regards,

Brian

···

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


#2

Hello Brian,

Brian Burch wrote:

Hi, Emil. I've had an item on my list of things to do for about 4
months and I think I might find some time to work on it fairly soon.

I don't want to commit a change until I've had your agreement on the
general idea.

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
net.java.sip.communicator.slick.protocol.icq.IcqTesterAgent.<init>(blah)
at
net.java.sip.communicator.slick.protocol.icq.IcqProtocolProviderSlick.start(blah)

Yes, this is indeed an old problem. Given its straightforward nature I was thinking that it represents an easy way for a newcomer to dive into the sip-communicator. I even remember a couple of people volunteering for it but I guess time constraints have prevented them from getting it done (no reproach implied).

I realise the user-oriented release of the sip-communicator will
ensure a suitable accounts.properties file is created, but that isn't
relevant to the unit tests.

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 presence states.

In other words, you have to enter 2 valid login id-s and passwords in this file and the tests would use them to go on-line with both of them and send messages from one to the other and vice versa.

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 this point.

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 online access).

or

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 wiki reference.

I personally believe that 1 is somewhat more logical but I don't mind 2) either.

What do you think?

Emil


#3

Thanks for taking the time to explain, Emil. You have saved me from taking a small innocent step and then being surprised by not getting the result I expected. Your description of the structure of the tests is very
clear and I'm sure will be helpful to others (as well as me).

At the moment, I am as unsure as you about which is the "best" of your two approaches. They both seem a bit sort-of-right and a bit sort-of-wrong, but in different ways. Given your warning, I prefer to spend more time
tracing and understanding the flow before I answer you. In the end, I'll favour an approach that is "most harmonious" with the existing code, but until I've understood the alternatives, I won't jump to conclusions.

Since posting my optimistic suggestion, a couple of urgent problems have turned up with my (paid) work. However, It looks like I'm not having to fight people off to make this change, so I'll try to do some research
later this week. I still won't make a change until I've discussed it further with you - unless one of the options turns out to be obviously better.
Regards,

Brian

···

On Mon, 19 Jun 2006 19:10:31 +0200, Emil Ivov wrote:

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
online access).

or

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
wiki reference.

I personally believe that 1 is somewhat more logical but I don't mind 2)
either.

What do you think?

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


#4

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
net.java.sip.communicator.slick.protocol.icq.IcqTesterAgent.<init>(blah)
at
net.java.sip.communicator.slick.protocol.icq.IcqProtocolProviderSlick.start(blah)

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
presence states.

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
this point.

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
online access).

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.

or

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
wiki reference.

I personally believe that 1 is somewhat more logical but I don't mind 2)
either.

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
unregister() calls.

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.

Regards,

Brian

···

On Mon, 19 Jun 2006 19:10:31 +0200, Emil Ivov wrote:

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


#5

Hi All,

This is a very profitable issue, I couldn't miss it....

First of all, the issue in question was assigned to me in the end of march, shame on me, I apologize!!!

By that time I manage to study the ANT <exec> task and we had discussed some of it in the list to, but since may we haven't talked about it since then. The account.properties file was the seconda task assigned to me, and this one I failed completely, althoug by your discussion, I think I wouldn't have contributed much anyway.... this time I realy apologize!!!! Sorry....

I have read some MSN documentation and also downloaded some MSN libraries, those Emil sugested. I also have looked the ICQ implementation, in order to plan the MSN protocol provider service implementation.

At the moment I thinking how would I test it, once I my opinion it is very important know how to test something before start developing it. That is way I became so glad when I read this topic!!!! Still, I have some doubts, not in the testing process, which I might have understood.

Here come the questions (ICQ protocol provide implementation questions will come in a different email.):
1) The package net.java.sip.communicator.slick.protocol.icq on the test folder became one test bundle, right?

2) What is the test agent??
3) What is the Slick Fixture???
4) Why use the System.getProperty() method and when was il set???
5) Is it possible to use the debbuger with the service installed in OSCAR??? How???

I better whait for those answers, they certainly will clear my thoughts and my understanding!!
Thanks and Best regards!!!!

Pedro

Brian Burch wrote:

···

On Mon, 19 Jun 2006 19:10:31 +0200, Emil Ivov wrote:

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 online access).

or

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 wiki reference.

I personally believe that 1 is somewhat more logical but I don't mind 2) either.

What do you think?
   
Thanks for taking the time to explain, Emil. You have saved me from taking a small innocent step and then being surprised by not getting the result I expected. Your description of the structure of the tests is very clear and I'm sure will be helpful to others (as well as me).

At the moment, I am as unsure as you about which is the "best" of your two approaches. They both seem a bit sort-of-right and a bit sort-of-wrong, but in different ways. Given your warning, I prefer to spend more time tracing and understanding the flow before I answer you. In the end, I'll favour an approach that is "most harmonious" with the existing code, but until I've understood the alternatives, I won't jump to conclusions.

Since posting my optimistic suggestion, a couple of urgent problems have turned up with my (paid) work. However, It looks like I'm not having to fight people off to make this change, so I'll try to do some research later this week. I still won't make a change until I've discussed it further with you - unless one of the options turns out to be obviously better.
Regards,

Brian

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

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


#6

Brian Burch wrote:

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.

I am happy! :slight_smile:

1. IcqProtocolProviderSlick.start() now detects the lack of a
properties file, so it knows there isn't any point running the normal
tests.

Good.

2. We can't run with zero tests, because the resulting html report
isn't specific enough.

Agree

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.

OK

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.

OK

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.

OK

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 unregister() calls.

OK

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.

Good catch! I agree.

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.

OK. Only, make sure (which I am sure you would have anyway) that the names of the private methods are as complete and as understandable as possible (even if it takes all the 80 columns). This SLICK has grown to be quite heavy so we should be careful to keep it readable.

Cheers
Emil


#7

Pedro Oliveira wrote:

Here come the questions (ICQ protocol provide implementation questions will come in a different email.):
1) The package net.java.sip.communicator.slick.protocol.icq on the test folder became one test bundle, right?

Yes. bundle-icq-slick . You can see the packaging of bundles in the build.xml
   

2) What is the test agent??

For the tests of ICQ you need two icq accounts. They are talking to each other, changing messages, changing statuses etc.
The one account is for the implementation itself the other one is for the Test Agent. Its simple and ugly impl of the icq actions he must take care of.

3) What is the Slick Fixture???

Fixure is simple class holding static fields with data - to be easy accessed from tests where needed.

4) Why use the System.getProperty() method and when was il set???

I think you mean for example the icq accounts are get with System.getProperty(
                TESTING_IMPL_ACCOUNT_ID_PROP_NAME, null)
This are set in build.xml for example
    <property file="lib/accounts.properties"/>

5) Is it possible to use the debbuger with the service installed in OSCAR??? How???

Yes it is possible its only matter of configuration of the IDE you are using.

I better whait for those answers, they certainly will clear my thoughts and my understanding!!
Thanks and Best regards!!!!

Pedro

I hope this answers are helpful

···

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


#8

I am happy! :slight_smile:

Pleased to be of service!

Good. >Agree >OK >OK >OK >OK >Good catch! I agree.

Watch this space... coming soon.

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.

OK. Only, make sure (which I am sure you would have anyway) that the
names of the private methods are as complete and as understandable as
possible (even if it takes all the 80 columns). This SLICK has grown to
be quite heavy so we should be careful to keep it readable.

Given the change will be to the start method of a test framework class, I'll give high priority to making the code simple to understand - even if it is less elegant or efficient.

Thanks for your quick answers - the source base is a rapidly moving target and I'll be pleased to commit my change and stop worrying about conflicts. Besides... I hope to be starting some (paid) development
work on Instant Messaging soon and I'd like to get my SipComm environement working properly, rather than just reproducing failures!!!
Regards,

Brian

···

On Fri, 07 Jul 2006 10:47:37 +0200, Emil Ivov wrote:

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