Github user osma commented on the pull request:
https://github.com/apache/jena/pull/53#issuecomment-109996262
Thanks for fixing, and sorry for causing the conflict with #72.
It's good that you've added unit tests, however I think there could be more
of them.
The current test adds and removes a resource, and only then checks that
it's gone. I think it should check that it got into the index in the first
place, otherwise it could be that text indexing is completely broken (no hits
ever) and the test would still pass.
Would it be possible/easy the structure the unit tests so that all regular
tests get executed also with the uid field enabled? After all, it shouldn't
affect the current functionality if you enable deletion support (if it does
it's a bug, either in implementation or the tests). You could get a lot of free
tests this way and there would perhaps be no need for further tests of
uid/deletion functionality.
A similar trick done with the graph-specific indexing, i.e. there are
general tests in AbstractTestDatasetWithTextIndex, then a couple of extra tests
for graph-aware functionality in AbstractTestDatasetWithGraphTextIndex, and
finally TestDatasetWithLuceneGraphTextIndex pulls it together with the right
(graph-aware) configuration. You could similarly try to reuse all the tests in
AbstractTestDatasetWithTextIndex for the uid case. I admit the class hierarchy
and naming is a bit complicated...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
