[sip-comm-dev] important change to unit testing properties


#1

I hope no-one is cursing me for my latest change to the way we use ./lib/testing.properties. Let me explain...

In the past, I've had a lot of difficulties with this file - sometimes I've accidentally committed my own local changes. Sometimes other people have done the same to me. I HOPE my latest change will stop this happening ever again.

I have implemented an analogous scheme to that already used with accounts.properties:

1. The repository now maintains a new file called ./lib/testing.properties.template. This file defines what we all hope is the most comprehensive set of test properties needed to run ALL unit test slicks.

2. The build.xml testing targets continue to use the file called ./lib/testing.properties to define their run-time properties.

3. The build.xml has been modified to verify the presence of a file called ./lib/testing.properties. This verification is ONLY performed when executing one of the unit testing targets. If the file does not exist on the local machine, then a helpful message is emitted and the run is terminated.

4. Each individual user MUST create a local file called ./lib/testing.properties. This is best done by copying the ./lib/testing.properties.template and then modifying it if necessary.

I apologise to anyone who is inconvenienced by my change. Emil and I discussed this strategy more than a year ago. I implemented it today because I rarely have time to work on the project and it was very relevant to the other changes I have been making.

.................................===> EMIL - PLEASE NOTE!!!!

5. Will my change break cruise control? If yes, would you please create a ./lib/testing.properties file that is appropriate to the maintenance server?

6. I have tried to keep the template file consistent with the last version of ./lib/testing.properties in the repository. I have changed the comments to make it easier to understand.

7. I do not understand why several slicks are not running as originally intended:-

7a. NetworkAddressManagerServiceLick currently sets up ZERO tests because the addSuite calls are commented-out in start().

7b. MediaServiceLick is commented-out of the testing.properties file.
When I run it locally on my own machine, one of the 2 tests fails.

7c. SipProtocolProviderServiceLick is commented-out of the testing.properties file. When I run it locally on my own machine, fourteen of the 25 tests fails.

7d. GenericProtocolProviderServiceLick is not defined in the testing.properties file. When I run it locally on my own machine, it fails because it does not add any tests to the suite.

7e. IcqProtocolProviderSlick is not defined in the testing.properties file. When I run it locally on my own machine, it fails because it does not add any tests to the suite.

If this is situation is "optimal" given the current state of the project, I think we should document the state of these slicks in the testing.properties.template file to prevent others from wasting unnecessary time investigating the missing tests.

Begards,

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

Hey Brian,

I was about to ping you on this one.

I am not sure I agree with this. Although some developers would like to
modify testing.properties so that it would only test the packages they
are working with, they should not be required to do so in order to start
testing. We are already having a relatively high testing barrier with
the accounts.properties template and I am quite reluctant to add an
extra one.

(more inline)

Brian Burch написа:

I hope no-one is cursing me for my latest change to the way we use
./lib/testing.properties. Let me explain...

In the past, I've had a lot of difficulties with this file - sometimes
I've accidentally committed my own local changes. Sometimes other people
have done the same to me. I HOPE my latest change will stop this
happening ever again.

This is true but I wouldn't say it's that much of a problem. I think
this is also what I told you when you accidentally committed your
version. Slips like this are easy to revert and they are not really
bothering anyone. In any case they are not preventing people from
running the tests so I'd prefer this to adding an extra step for
newcomers trying to start testing.

I apologise to anyone who is inconvenienced by my change. Emil and I
discussed this strategy more than a year ago. I implemented it today
because I rarely have time to work on the project and it was very
relevant to the other changes I have been making.

Not sure I remember this discussion, but in any case I apologise if I've
misled you and made you think that I agree.

.................................===> EMIL - PLEASE NOTE!!!!

5. Will my change break cruise control? If yes, would you please create
a ./lib/testing.properties file that is appropriate to the maintenance
server?

Yes, although this is not what bothers me most.

6. I have tried to keep the template file consistent with the last
version of ./lib/testing.properties in the repository. I have changed
the comments to make it easier to understand.

7. I do not understand why several slicks are not running as originally
intended:-

Mostly because they were failing for reasons that we couldn't address at
the time. It might be worth looking at them again.

If this is situation is "optimal" given the current state of the
project, I think we should document the state of these slicks in the
testing.properties.template file to prevent others from wasting
unnecessary time investigating the missing tests.

I agree. We should definitely add the necessary comments in
testing.properties once we revert to it.

Cheers
Emil

···

Begards,

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


#3

Emil Ivov wrote:

Hey Brian,

I was about to ping you on this one.

I am not sure I agree with this. Although some developers would like to
modify testing.properties so that it would only test the packages they
are working with, they should not be required to do so in order to start
testing. We are already having a relatively high testing barrier with
the accounts.properties template and I am quite reluctant to add an
extra one.

Sorry to have been too hasty. I will put things right as soon as you tell me what you think is the best approach.

This matter is a problem for me because I do not have a collection of jabber/MSN/etc acccounts that I can use for testing. I have to bypass these test suites if I want to run a successful "test all".

What if I were to add an extra twist?

Instead of failing when testing.properties is not found, would it be acceptable to copy the template file to create a default local file?

That would help a new developer get started with a clean sandbox but, of course, it would not propagate any subsequent changes made to the template file.

What do you think?

Brian

···

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


#4

Hey Brian,

We actually have the accounts.icq.DISABLE_ONLINE_TESTING in
accounts.properties(.template) and that's exactly why we implemented it.

Would this do the deal?

Cheers
Emil

Brian Burch написа:

···

Emil Ivov wrote:

Hey Brian,

I was about to ping you on this one.

I am not sure I agree with this. Although some developers would like to
modify testing.properties so that it would only test the packages they
are working with, they should not be required to do so in order to start
testing. We are already having a relatively high testing barrier with
the accounts.properties template and I am quite reluctant to add an
extra one.

Sorry to have been too hasty. I will put things right as soon as you
tell me what you think is the best approach.

This matter is a problem for me because I do not have a collection of
jabber/MSN/etc acccounts that I can use for testing. I have to bypass
these test suites if I want to run a successful "test all".

What if I were to add an extra twist?

Instead of failing when testing.properties is not found, would it be
acceptable to copy the template file to create a default local file?

That would help a new developer get started with a clean sandbox but, of
course, it would not propagate any subsequent changes made to the
template file.

What do you think?

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


#5

At the moment, cruise control seems to have held on to its local copy of testing.properties, so I think there is no need for me to panic. Let us try to sort the situation out carefully but fairly quickly.

Emil Ivov wrote:

We actually have the accounts.icq.DISABLE_ONLINE_TESTING in
accounts.properties(.template) and that's exactly why we implemented it.

Would this do the deal?

I just checked the current template. It currently has the following...

# if for some reason you want not to run online icq tests (e.g. because they
# take an awful lot to complete). set this property to true (optional)
# accounts.icq.DISABLE_ONLINE_TESTING

In general, I am not keen on double-negatives, but I agree with your choice of property name given that you want the default case to be to RUN the tests.

Why did you not explicitly code the default case uncommented as follows?:

accounts.icq.DISABLE_ONLINE_TESTING=false

or commented like this?:
# accounts.icq.DISABLE_ONLINE_TESTING=true

···

----------------------------------------------

Now to the general case... This kind of control does not currently exist for the MSN, Jabber and Yahoo test suites.

I think we are heading in a useful direction...

Let us say that we prefer to avoid the testing.properties.template scheme because it is too complex for most users to get right.

This would mean we will define the testing.properties list of slicks to mean "all slicks that are expected to run successfully when proper accounts.properties have been defined".

Then it would be logical for the accounts.properties entries associated with a particular "optional" protocol to be controlled by a similar "master switch" to the icq case...

e.g. accounts.jabber.DISABLE_ONLINE_TESTING=false

I realise this means adding some additional control logic to each of the slicks, but it probably wouldn't take much longer than typing this email!

--------------------------------------

If you agree with my logic this far, why not untangle the double negative as part of the change? In other words, let the template explicitly define...

accounts.icq.ENABLE_ONLINE_TESTING=false

This should be obvious enough to anyone setting up their own accounts.properties from the template, especially if it is the first property in the block.

You must explicitly enable the group of protocol tests. There is no point enabling them until you have defined your own associated protocol properties!

What do you think so far?

Brian

---------------------------------------------------------------------
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 написа:

At the moment, cruise control seems to have held on to its local copy of
testing.properties,

Yes I reverted the local copy on CC so no worries.

Why did you not explicitly code the default case uncommented as follows?:

accounts.icq.DISABLE_ONLINE_TESTING=false

or commented like this?:
# accounts.icq.DISABLE_ONLINE_TESTING=true

No particular reason. Feel free to change if this bothers you.

Now to the general case... This kind of control does not currently exist
for the MSN, Jabber and Yahoo test suites.

Possible, we could easily add it though.

I think we are heading in a useful direction...

Let us say that we prefer to avoid the testing.properties.template
scheme because it is too complex for most users to get right.

Glad we agree.

This would mean we will define the testing.properties list of slicks to
mean "all slicks that are expected to run successfully when proper
accounts.properties have been defined".

Then it would be logical for the accounts.properties entries associated
with a particular "optional" protocol to be controlled by a similar
"master switch" to the icq case...

e.g. accounts.jabber.DISABLE_ONLINE_TESTING=false

I realise this means adding some additional control logic to each of the
  slicks, but it probably wouldn't take much longer than typing this email!

Agreed.

If you agree with my logic this far, why not untangle the double
negative as part of the change? In other words, let the template
explicitly define...

accounts.icq.ENABLE_ONLINE_TESTING=false

The reason why the property was initially named DISABLE is because we
generally see such variables as flags that should be raised (i.e. set to
true) when you want to change the default behaviour.

On the other hand I am now thinking that we should probably have offline
tests as the default. This would make it even easier to setup the tests.

Then we could even think about making the accounts.properties file
optional and only necessary when you explicitly want online testing ...
but this might require a bit more work than a simple hack so we might
want to leave it for later.

You must explicitly enable the group of protocol tests. There is no
point enabling them until you have defined your own associated protocol
properties!

Oh I didn't actually see this part when I first read the mail ... we
appear to agree then. :slight_smile:

What do you think so far?

I think all this sounds reasonable and we can go ahead.

Cheers
Emil

···

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


#7

Emil Ivov wrote:
<BIG-snip>

I think all this sounds reasonable and we can go ahead.

Firstly, when I checked my local accounts.properties, I was surprised to discover that I actually owned a set of test accounts and also (once upon a time!) intended to run the icq test suite!

Then I reinstated the IcqProtocolProviderSlick line in my local testing.properties...
... but the test failed! I looked at the latest cruise control log and (if I can believe my eyes) the icq slick is NOT being run.

I've just added "reference:file:sc-bundles/protocol-icq-slick.jar" to felix.unit.test and my icq test suite runs properly (although 2 of the 35 tests fail).

Can I commit this change to felix.unit.test asap, or did you mean to inhibit that suite under CC?

···

-------------------------------------

I'll be away tomorrow, so I'll try to make the property semantics change to IcqProtocolProviderSlick over the weekend.

I'll also revert my testing.properties.template change in as positive manner as possible (i.e. not make a trivial backout to an earlier version). That should mean that everyone can go back to running with the repository copy of the file.

I'll then update the wiki to reflect this discussion.

(That leaves MSN, jabber and yahoo slicks to get the same treatment in the fullness of time).

---------

Thanks for your speedy and thoughtful comments.

Brian

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


#8

Brian Burch написа:

Emil Ivov wrote:
Can I commit this change to felix.unit.test asap, or did you mean to
inhibit that suite under CC?

Yes, they were one of the tests that we didn't have a quick work around
for. Although we could probably bring back all those that pass.

-------------------------------------

I'll be away tomorrow, so I'll try to make the property semantics change
to IcqProtocolProviderSlick over the weekend.

OK, thanks.

Thanks for your speedy and thoughtful comments.

Thank _you_ for opening the discussion, Brian. I like the solution that
we've agreed on.

Cheers
Emil

···

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