search

shrinkwrap / descriptors

ShrinkWrap subproject for creating Archive Descriptors
Apache License 2.0
26 stars 30 forks source link

Shrinkdesc 54 #27

Closed rbattenfeld closed 12 years ago

rbattenfeld commented 12 years ago

Hi Andrew

Finally I managed the push the changes to my SHRINKDESC-54 branch:-)

Yesterday, it took my along time to realize that the parent pom version has changed.

Let me describe what I have done so far:

  1. All classes have the copyright and author comments. These comments are not hard coded. There are two new xml files in the gen/src/main/resources/xslt for maintaining these information.
  2. All classes and methods are commented with javadoc as well as the packages. The gen/doc folder contains the manually generated javadoc html files.
  3. Junit test coverage. I checked the unit test coverage with eCobertuna and found some methods for which the test case generation step didn't write test cases. This is fixed. Overall, the unit test coverage is about 95 %. Please be aware that the generated classes are tested inside the gen module because the generator writes also all the test cases. If you wish we could move the generated test cases to the impl module.
  4. I included also a test that verifies that null values are not causing a null pointer exception. For example, returning an integer value by Integer.parseValue() will throw an exception. Such things shouldn't happen.
  5. I think the state is good but I am very open, the real test is when the descriptor is generating descriptor files for real deployments. I didn't had the time to do such test and believe this is better to test with an alpha version in a public repository.

The next thing I will start is thinking about the wiki you have mentioned last week.

Thanks again for your help. Ralf

ALRubinger commented 12 years ago

upstream/SHRINKDESC-54: https://github.com/shrinkwrap/descriptors/commit/2e7b639b9a1a582c9592c66e51fe0e72d080d1b8

ALRubinger commented 12 years ago

1) Great! 2) Also great, so long as the generated documentation is useful/sufficient. Looks so (ie. noting the XML structure the model represents) 3) Yes, let's now move the test cases into impl. 4) Where's this test? And in what context? For example, if null values are not really permitted to be input to the API, this should be documented and then guarded by a precondition check that throws something like new IllegalArgumentException("null value given for X, disallowed") 5) Sure, we'll try this out in some real downstream projects. But at the "Descriptors" level, all of our tests should be strong enough that our assertions are holding as expected.

Great stuff! I'm loving how this is coming together.

rbattenfeld commented 12 years ago

Hi Andrew

Sorry for the late answer. I was busy in the office over last days.

4) The null pointer test is located in the class TestDescriptorImpl. There is reflection based method that is applied for all generated class. The issues was not to pass null values, the issues was for the getXXX operations and the element is not initialized. In this case, the node implementation returns "null", which caused an NumberFormatException for some of the methods which returns Integer or Long values. So, I think, it is a good test.

5) Yes I agree. There are a couple of test files which are testing the schema compatibility. I copied all the existing test cases and adjusted these test cases to the new descriptors, where possible.

I also love this project. I saw that there new commits for the spi layer. Cool! When I see you the next time, I have a couple of questions regarding the wiki we talked about.