On 12/28/2012 07:34 PM, Andrea Aime wrote: > Hi, > I did not see a review from Jody so far, so I've tried to make one myself > instead (and seen that Rini made a review for the app-schema part last > week). > > I did not do a line by line review, but from what I see things are > looking good. > > I do have a couple of concerns though, but I don't see them as > blocking the > merge of the patch. > > The first one is testing related: the module complex module has a > low-ish test > coverage (29%), and all the classes contributed to "main" are without any > kind of test. > Now, from what I understand your work has been mostly refactoring of > existing code, so I certainly cannot blame you for code coverage that was > not there to begin with, or for coverage that was there when things were > all togheter in a single module, and that is no more visible due to > the split, > yet the classes I see there seem testable in isolation... > Complex feature people, any comment?
I had noticed this myself, and had already expected this concern. I suggest that a separate task is created to improve complex feature test base, but I wanted to keep this patch strictly about refactoring. In other words, the changes are completely neutral with regards to test coverage. I think the reason might be that because of the existence of the app-schema-test package in geoserver there has been traditionally less test coverage in the gt-app-schema package itself, but now it becomes clear that some basic complex feature classes do need to be tested on their own behaviour. In a few cases the problem is also that some tests had to remain in app-schema even if the actual classes moved. For example ComplexAttributeConverterFactory, this class could perfectly be moved to gt-main but its current test class is totally app-schema based. The same issue with the FeatureRegistry that moved to gt-complex. New tests should be made that are more generic. However I do think that most stuff in gt-complex is in fact tested, even if there seems to be way less test classes than actual classes, don't forget the FeaturePropertyAccessorTest tests the whole bunch of x-path classes (which only work together). > The other thing I'm working about is the xpath property accessor, I see > there is another one in xsd-core, which is something that gt-complex > depends > onto. > What is the relationship between the two? Is the duplication necessary? The ones in xsd-core work only on SimpleFeatures. I never really understood their use, why would you need an x-path on a simple feature anyway? In any case, the complex x-path accessor should work on simple features too as a special case. In my opinion this can be completely removed. Kind Regards Niels Niels ------------------------------------------------------------------------------ Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS, MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft MVPs and experts. SALE $99.99 this month only -- learn more at: http://p.sf.net/sfu/learnmore_122412 _______________________________________________ GeoTools-Devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
