stardog-union / pellet

Pellet is an OWL 2 reasoner in Java; open source (AGPL) and commercially licensed, commercial support available.
http://clarkparsia.com/pellet
Other
302 stars 153 forks source link

Updated tests in maven branch #5

Closed tobias-hammerschmidt closed 10 years ago

tobias-hammerschmidt commented 10 years ago

Hi @evren, I've updated the test cases in the maven branch of pellet. In essence the changes to the test classes from @ansell are reverted and the maven surefire plugin is configured to set the correct working directory as expected by the test cases. That means there is no need any longer to copy resources from classpath to file locations as discussed in https://github.com/clarkparsia/pellet/pull/3 and the test classes can also be reused as is. I also removed the StableTests and UnstableTests categories as well as the test profiles in the parent pom. Note that the test cases still need some review:

Would be nice if we can sort out the remaining issues (test cases and assembly configuration) so the maven migration can be finished soon.

ansell commented 10 years ago

Sorry for not getting back to it to finish the migration. It is a big job but I didn't think that it was so far broken in its current state to need to revert it all and then have to start again later. Can you enumerate the changes that are still required if we did not revert all of my changes to the test classes so far and why it wouldn't be easier just to continue on with what I had started to get it completed and operating using the standard maven way?

tobias-hammerschmidt commented 10 years ago

Hi @ansell let me first thank you for all the effort you already spent on this - really appreciated. The reason for reverting the test case changes are pragmatic only - we want the maven migration to be finished ASAP. I wouldn't desribe the current state as broken but the fact that still workarounds like copying resources from classpath to a temporary directory are needed makes me wonder whether this is worth the effort. And file based access can't be circumvented for all cases anyway as also pointed out by @evren in #3. I think setting the working directory in the surefire plugin isn't violating the maven way. The test cases can still be gradually improved. I also like your test case improvements like setting a timeout or grouping long running tests in maven profiles. If upstream also agrees to these changes they should really be incorporated from my POV. I can't really answer which test cases still need to be converted in your last revision (and again you can't 'fix' all properly - look for instance at test/src/test/resources/data/trans-tree-tests/discovery.owl which imports a relative file resource - would be really hard to map this to some known location in a portable way without preprocessing). Assuming that we end up in a mixture of improved test case classes with file url based access (same as in current master) required changes from my POV include:

evren commented 10 years ago

I agree with Tobias here. I'll merge this pull request so that we can finalize the mavenization quickly and then we can look at improving the tests later.