-
Notifications
You must be signed in to change notification settings - Fork 33
Improve handling of ad-hoc OBO namespaces in ShortFormAnnotator #937
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
|
I wasn't able to find a place to write some unit tests - @haideriqbal could you advise where I can put some? |
|
@cthoyt we don't have unit tests in OLS, we do manual testing at the moment of comparing the dataload files for different test scenarios... for this change i will need to do that and will let you know... we are looking into automating the testcases as some point |
|
@haideriqbal I added some unit tests, but I'm not sure the best way to run them/incorporate in CI |
|
@cthoyt we don't run unit test in traditional sense for OLS. We have a testcases folder where we add specific kind of ontologies to address various OLS use cases. If you can have a look at the folder and formulate a test which depicts this change that will be awesome. Also, have a read through OLS readme to get a gist of how we run the testcases for dataload in OLS. |
|
hi @haideriqbal I started going down the road of adding some end-to-end tests but found that it was pretty inaccessible as an outside developer. Instead, I got the unit tests to work as part of the build, which can also be built with However, it might be the case that improving the short form annotator invalidated other end-to-end tests, and I'm having a hard time seeing if that's the case since they don't give very pointed output (i.e., there's way too much logging, I can't see what is important) |
|
@haideriqbal I'd appreciate a second look at this - I can confirm that The test failures are likely unrelated, and are also failing on the main branch, but it's also not 100% certain that there are not subtle changes introduced here that would also need to be updated in the unit tests |
|
@cthoyt test failures are not related to your PR... it's the way the tests are designed at the moment for OLS. We do manual testing where we compare the result of dataload part of the pipeline and then also run API testing. Details of these two methods are written in our README. The issue is as this is done via file comparison and almost everytime we have false positive where pipeline writes some json object at a different place then where it was before which results in a mismatch and test fails. This situation is not suited for automated testing hence we have failures everytime in the github actions. Until unless, we change the way we test OLS I personally don't see a point of having automated testing where its going to fail everytime. |
Related to #935
This PR extends
ShortFormAnnotator.javato handle ad-hoc namespaces in the OBO PURL space that take the formhttp://purl.obolibrary.org/obo/<lower>#<luid>, such as http://obolibrary.org/obo/mesh#CThis PR will give more accurate short form names that does not rely on implicit capitalization conventions