Patches Need Tests

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Patches Need Tests

Christopher Schmidt-2
Recently, we've been getting, and applying a large number of patches
from the community. These patches have helped to solve dozens of bugs
that might have otherwise gone undiscovered in the OpenLayers code base.

However, our testing of these changes has fallen by the wayside. In the
past, I took the time out to write tests for most of the patches that I
committed -- anything that could be tested programatically. Recently, I
haven't taken this time out: patches have been committed which have no
tests, and this is the kind of thing that leads to regressions.

I'd like to see more patches accompanied by tests. Understandably, not
all problems can be easily tested in an automated way. An example of
this is the init tiles issue, #480. I was unable to come up with an easy
way to write an automated test for this, so instead, I wrote a simple
HTML page which demonstrated the problem, with a description of what
should be seen and what shouldn't:

  http://openlayers.org/dev/tests/grid_inittiles.html

I understand that there are cases where this is the case, and I would be
much happier accepting a patch if it had even a non-automated test that
described the problem it was solving, and how to reproduce it, and what
it should look like if it has passed or failed.

I'm speaking solely for myself here, but I'm leaning towards pushing
back on patch writers to write some kind of tests that go along with
patches where possible. An example of this is ticket #496. In that case,
I was able to write tests:

  http://trac.openlayers.org/changeset/2237

In the future, I'd like to see patch authors include these tests where
possible.

Is there anything that can be done to make the test-writing process
simpler? Do we need more docs on the test writing stuff, or is this just
something people hadn't been thinking about?

Regards,
--
Christopher Schmidt
MetaCarta
_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: Patches Need Tests

Cameron Shorter
Chris,
I think that your guidelines should specify that if someone provides a
patch, then they should also provide a patch for the tests for that
file. It would help if there is a "Writing Unit Tests" at
http://trac.openlayers.org/wiki/CodingStandards has some more detail:
* Note the JS tool being used for testing.
* Point to the tool's documentation explaining how to write tests
* Probably write a short howto explaining what is expected from users.
 I'd expect to see writing Unit Tests added as a requirement to "Upload
your patch" linked from http://trac.openlayers.org/wiki/HowToContribute

Unit testing in OpenLayers helps ensures a level of quality which
OpenLayers has become known for. I think it is something that is
relatively easy to sell to contributors.
(I realize my track record with writing OL UTs is poor, but doesn't stop
me believing they should be mandated).

Do you testing tools provide you with a code coverage report?

Christopher Schmidt wrote:

> Recently, we've been getting, and applying a large number of patches
> from the community. These patches have helped to solve dozens of bugs
> that might have otherwise gone undiscovered in the OpenLayers code base.
>
> However, our testing of these changes has fallen by the wayside. In the
> past, I took the time out to write tests for most of the patches that I
> committed -- anything that could be tested programatically. Recently, I
> haven't taken this time out: patches have been committed which have no
> tests, and this is the kind of thing that leads to regressions.
>
> I'd like to see more patches accompanied by tests. Understandably, not
> all problems can be easily tested in an automated way. An example of
> this is the init tiles issue, #480. I was unable to come up with an easy
> way to write an automated test for this, so instead, I wrote a simple
> HTML page which demonstrated the problem, with a description of what
> should be seen and what shouldn't:
>
>   http://openlayers.org/dev/tests/grid_inittiles.html
>
> I understand that there are cases where this is the case, and I would be
> much happier accepting a patch if it had even a non-automated test that
> described the problem it was solving, and how to reproduce it, and what
> it should look like if it has passed or failed.
>
> I'm speaking solely for myself here, but I'm leaning towards pushing
> back on patch writers to write some kind of tests that go along with
> patches where possible. An example of this is ticket #496. In that case,
> I was able to write tests:
>
>   http://trac.openlayers.org/changeset/2237
>
> In the future, I'd like to see patch authors include these tests where
> possible.
>
> Is there anything that can be done to make the test-writing process
> simpler? Do we need more docs on the test writing stuff, or is this just
> something people hadn't been thinking about?
>
> Regards,
>  


--
Cameron Shorter
Systems Architect, http://lisasoft.com.au
Tel: +61 (0)2 8570 5011
Mob: +61 (0)419 142 254

_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: Patches Need Tests

Christopher Schmidt-2
On Sun, Feb 18, 2007 at 07:07:56AM +1100, Cameron Shorter wrote:
> Chris,
> I think that your guidelines should specify that if someone provides a
> patch, then they should also provide a patch for the tests for that
> file. It would help if there is a "Writing Unit Tests" at
> http://trac.openlayers.org/wiki/CodingStandards has some more detail:
> * Note the JS tool being used for testing.
> * Point to the tool's documentation explaining how to write tests
> * Probably write a short howto explaining what is expected from users.

http://trac.openlayers.org/wiki/CodingStandards now links to
http://trac.openlayers.org/wiki/WritingUnitTests , which sounds like 1
and 2 of above. I'd gladly take feedback on what 3. should be :)

> I'd expect to see writing Unit Tests added as a requirement to "Upload
> your patch" linked from http://trac.openlayers.org/wiki/HowToContribute

I've put it in http://trac.openlayers.org/wiki/CreatingPatches which is
linked from that page.

> Do you testing tools provide you with a code coverage report?

I think the answer to this is 'no'. I don't have any clue how you would
create such a report in the Javascript environment.

Regards,
--
Christopher Schmidt
MetaCarta
_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: Patches Need Tests

Cameron Shorter
Nice,
I believe you have covered all my suggestions.

Christopher Schmidt wrote:

> On Sun, Feb 18, 2007 at 07:07:56AM +1100, Cameron Shorter wrote:
>  
>> Chris,
>> I think that your guidelines should specify that if someone provides a
>> patch, then they should also provide a patch for the tests for that
>> file. It would help if there is a "Writing Unit Tests" at
>> http://trac.openlayers.org/wiki/CodingStandards has some more detail:
>> * Note the JS tool being used for testing.
>> * Point to the tool's documentation explaining how to write tests
>> * Probably write a short howto explaining what is expected from users.
>>    
>
> http://trac.openlayers.org/wiki/CodingStandards now links to
> http://trac.openlayers.org/wiki/WritingUnitTests , which sounds like 1
> and 2 of above. I'd gladly take feedback on what 3. should be :)
>
>  
>> I'd expect to see writing Unit Tests added as a requirement to "Upload
>> your patch" linked from http://trac.openlayers.org/wiki/HowToContribute
>>    
>
> I've put it in http://trac.openlayers.org/wiki/CreatingPatches which is
> linked from that page.
>
>  
>> Do you testing tools provide you with a code coverage report?
>>    
>
> I think the answer to this is 'no'. I don't have any clue how you would
> create such a report in the Javascript environment.
>
> Regards,
>  


--
Cameron Shorter
Systems Architect, http://lisasoft.com.au
Tel: +61 (0)2 8570 5011
Mob: +61 (0)419 142 254

_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: Patches Need Tests

Paul Spencer-2
In reply to this post by Christopher Schmidt-2
Chris,

I recommend putting in something about requiring tests to be run  
against the major browsers - at least IE 6/7 and FireFox :)

Cheers

Paul

On 18-Feb-07, at 8:42 AM, Christopher Schmidt wrote:

> On Sun, Feb 18, 2007 at 07:07:56AM +1100, Cameron Shorter wrote:
>> Chris,
>> I think that your guidelines should specify that if someone  
>> provides a
>> patch, then they should also provide a patch for the tests for that
>> file. It would help if there is a "Writing Unit Tests" at
>> http://trac.openlayers.org/wiki/CodingStandards has some more detail:
>> * Note the JS tool being used for testing.
>> * Point to the tool's documentation explaining how to write tests
>> * Probably write a short howto explaining what is expected from  
>> users.
>
> http://trac.openlayers.org/wiki/CodingStandards now links to
> http://trac.openlayers.org/wiki/WritingUnitTests , which sounds like 1
> and 2 of above. I'd gladly take feedback on what 3. should be :)
>
>> I'd expect to see writing Unit Tests added as a requirement to  
>> "Upload
>> your patch" linked from http://trac.openlayers.org/wiki/ 
>> HowToContribute
>
> I've put it in http://trac.openlayers.org/wiki/CreatingPatches 
> which is
> linked from that page.
>
>> Do you testing tools provide you with a code coverage report?
>
> I think the answer to this is 'no'. I don't have any clue how you  
> would
> create such a report in the Javascript environment.
>
> Regards,
> --
> Christopher Schmidt
> MetaCarta
> _______________________________________________
> Dev mailing list
> [hidden email]
> http://openlayers.org/mailman/listinfo/dev

+-----------------------------------------------------------------+
|Paul Spencer                          [hidden email]    |
+-----------------------------------------------------------------+
|Chief Technology Officer                                         |
|DM Solutions Group Inc                http://www.dmsolutions.ca/ |
+-----------------------------------------------------------------+




_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev
Reply | Threaded
Open this post in threaded view
|

Re: Patches Need Tests

John Cole X
In reply to this post by Christopher Schmidt-2
As someone who lives by unit tests, I wholeheartedly endorse this.  I
require my developers to include unit tests before passing a code review
(otherwise how does the reviewer know the patch works).  While I haven't had
the time just yet to delve into OpenLayers code just yet, instructions for
performing testing on JS apps like OL would be fantastic, and something that
I would take advantage of when coding JS using OL.

And when a unit test won't work, functional examples should be included and
the instructions for doing so should explain how to create these as well.
Since it seems the team has done both in the past, hopefully it won't be too
difficult to create the documentation for this.

John

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On
Behalf Of Cameron Shorter
Sent: Saturday, February 17, 2007 2:08 PM
To: Christopher Schmidt
Cc: [hidden email]
Subject: Re: [OpenLayers-Dev] Patches Need Tests

Chris,
I think that your guidelines should specify that if someone provides a
patch, then they should also provide a patch for the tests for that
file. It would help if there is a "Writing Unit Tests" at
http://trac.openlayers.org/wiki/CodingStandards has some more detail:
* Note the JS tool being used for testing.
* Point to the tool's documentation explaining how to write tests
* Probably write a short howto explaining what is expected from users.
 I'd expect to see writing Unit Tests added as a requirement to "Upload
your patch" linked from http://trac.openlayers.org/wiki/HowToContribute

Unit testing in OpenLayers helps ensures a level of quality which
OpenLayers has become known for. I think it is something that is
relatively easy to sell to contributors.
(I realize my track record with writing OL UTs is poor, but doesn't stop
me believing they should be mandated).

Do you testing tools provide you with a code coverage report?

Christopher Schmidt wrote:

> Recently, we've been getting, and applying a large number of patches
> from the community. These patches have helped to solve dozens of bugs
> that might have otherwise gone undiscovered in the OpenLayers code base.
>
> However, our testing of these changes has fallen by the wayside. In the
> past, I took the time out to write tests for most of the patches that I
> committed -- anything that could be tested programatically. Recently, I
> haven't taken this time out: patches have been committed which have no
> tests, and this is the kind of thing that leads to regressions.
>
> I'd like to see more patches accompanied by tests. Understandably, not
> all problems can be easily tested in an automated way. An example of
> this is the init tiles issue, #480. I was unable to come up with an easy
> way to write an automated test for this, so instead, I wrote a simple
> HTML page which demonstrated the problem, with a description of what
> should be seen and what shouldn't:
>
>   http://openlayers.org/dev/tests/grid_inittiles.html
>
> I understand that there are cases where this is the case, and I would be
> much happier accepting a patch if it had even a non-automated test that
> described the problem it was solving, and how to reproduce it, and what
> it should look like if it has passed or failed.
>
> I'm speaking solely for myself here, but I'm leaning towards pushing
> back on patch writers to write some kind of tests that go along with
> patches where possible. An example of this is ticket #496. In that case,
> I was able to write tests:
>
>   http://trac.openlayers.org/changeset/2237
>
> In the future, I'd like to see patch authors include these tests where
> possible.
>
> Is there anything that can be done to make the test-writing process
> simpler? Do we need more docs on the test writing stuff, or is this just
> something people hadn't been thinking about?
>
> Regards,
>  


--
Cameron Shorter
Systems Architect, http://lisasoft.com.au
Tel: +61 (0)2 8570 5011
Mob: +61 (0)419 142 254

_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev

--
No virus found in this incoming message.
Checked by AVG Free Edition.
Version: 7.5.441 / Virus Database: 268.18.1/690 - Release Date: 2/16/2007
2:25 PM
 

--
No virus found in this outgoing message.
Checked by AVG Free Edition.
Version: 7.5.441 / Virus Database: 268.18.2/692 - Release Date: 2/18/2007
4:35 PM
 
This email and any files transmitted with it are confidential and intended solely for the use of the individual or entity to whom they are addressed. If you have received this email in error please notify the sender. This message contains confidential information and is intended only for the individual named. If you are not the named addressee you should not disseminate, distribute or copy this e-mail.
_______________________________________________
Dev mailing list
[hidden email]
http://openlayers.org/mailman/listinfo/dev