Hmmm okay perhaps you're right in theory, but I just think that even
though the behaviour of the end result is supposed to be the same, this
method alters the internal behaviour a lot and adds a very specific
procedure to the mix (creating a filter from a tree) that I normally
would have expected to see individually tested. I often get asked to
make tests for much more trivial things that that, I assumed every
significant change needs to be tested, and I thought that integration
tests are not sufficient but that new classes and significant methods
need to be tested as well. But perhaps I am mistaken.
Regards
Niels
On 18-03-15 16:55, Andrea Aime wrote:
org.geoserver.security.impl.On Wed, Mar 18, 2015 at 3:35 PM, Niels
Charlier <[email protected] <mailto:[email protected]>> wrote:
That's cool, but I think in the first place there should be a simple
test (it does not need many layers) that merely confirms that the
conversion from tree to filter is being done correctly. At the moment
there is no such test. This new method has been added and became
part of
the security system of geoserver; when this very crucial change
was made
a test should have been added that proves the correctness of this
method, which unfortunately didn't happen.
So far we had rules to add tests for new features, and bug fixes.
This work has been a performance optimization, no expected behavioral
changes, and it used the pre-existing
integration test coverage to confirm its correctness.
So, you're basically saying that when performance optimization is done,
with no intent to change behavior, tests should be added to cover any
replacement code path, ignoring the pre-existing coverage, of course,
assuming there is already a good amount of it, like in this case.
I think I'm good with the direction, but we need a clear agreement,
since that drives what
we ask contributors to do when we receive pull requests (like, I don't
want different core devs to give different advices on this on different
pull requests, or worse, in the same pull request, confusing the
contributor).
Cheers
Andrea
PS: to give everybody some context, here are the pre-existing test classes
that end up invoking the method in question, and checking its behavior
from an integration testing point of view:
* org.geoserver.security.impl.SecureCatalogImplTest
* org.geoserver.security.impl.SecureCatalogIntegrationTest
I thought wms/wcs/wfs also had tests covering the
DefaultResourceAccessManager,
but in fact the are using a mock one
--
==
GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.
==
Ing. Andrea Aime
@geowolf
Technical Lead
GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313 <tel:%2B39%200584%20962313>
fax: +39 0584 1660272 <tel:%2B39%200584%201660272>
mob: +39 339 8844549 <tel:%2B39%20%C2%A0339%208844549>
http://www.geo-solutions.it
http://twitter.com/geosolutions_it
*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*
Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate.
Il loro utilizzo è consentito esclusivamente al destinatario del
messaggio, per le finalità indicate nel messaggio stesso. Qualora
riceviate questo messaggio senza esserne il destinatario, Vi preghiamo
cortesemente di darcene notizia via e-mail e di procedere alla
distruzione del messaggio stesso, cancellandolo dal Vostro sistema.
Conservare il messaggio stesso, divulgarlo anche in parte,
distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità
diverse, costituisce comportamento contrario ai principi dettati dal
D.Lgs. 196/2003.
The information in this message and/or attachments, is intended solely
for the attention and use of the named addressee(s) and may be
confidential or proprietary in nature or covered by the provisions of
privacy act (Legislative Decree June, 30 2003, no.196 - Italy's New
Data Protection Code).Any use not in accord with its purpose, any
disclosure, reproduction, copying, distribution, or either
dissemination, either whole or partial, is strictly forbidden except
previous formal approval of the named addressee(s). If you are not the
intended recipient, please contact immediately the sender by
telephone, fax or e-mail and delete the information in this message
that has been received in error. The sender does not give any warranty
or accept liability as the content, accuracy or completeness of sent
messages and accepts no responsibility for changes made after they
were sent or for other risks which arise as a result of e-mail
transmission, viruses, etc.
-------------------------------------------------------
------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Geoserver-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
