added fix for conversion between SBOL2 and SBOL3 #90
Conversation
test/test_conversion.py
Outdated
| for c in doc2.componentDefinitions: | ||
| for sa in c.sequenceAnnotations: | ||
| for loc in sa.locations: | ||
| assert loc.orientation != 'http://sbols.org/v3#inline' |
There was a problem hiding this comment.
Can we also have a positive check that we do have some SBOL2 inline marks where expected? Orientation is optional, and so we need to make sure we're actually exercising the test - I don't know if the test file has an orientation in it at all.
|
This is looking good; I'd like to see a strengthening of the test case, since I'm not sure if the test file has an orientation listed at all. |
|
Hi @jakebeal, I have made few changes:
|
There was a problem hiding this comment.
Thank you, nice progress. Just a couple of small tweaks to request in the code.
With regards to this:
for the actual testing test_files/BBa_J23101.nt file which is being used in test_3to2_conversion function does not contain location block with orientation. For testing the changes I used test_files/iGEM_SBOL2_imports.nt file through python's interactive shell.
I'm glad that it worked for your local test. We won't know if it's still working once somebody else changes the code, however, unless we add a test to the test suite. Can you add a new test that uses test_files/iGEM_SBOL2_imports.nt? Tests are cheap and bugs are expensive!
test/test_conversion.py
Outdated
| @@ -62,10 +63,23 @@ def test_3to2_conversion(self): | |||
| assert doc2.componentDefinitions[0].sequences[0] == 'https://synbiohub.org/public/igem/BBa_J23101_sequence' | |||
| assert doc2.sequences[0].encoding == 'http://www.chem.qmul.ac.uk/iubmb/misc/naseq.html' | |||
| assert doc2.sequences[0].elements == 'tttacagctagctcagtcctaggtattatgctagc' | |||
| # ids of location block containing orientation before conversion | |||
| location_ids_sbol3 = [] | |||
| def change_if_location(o): | |||
There was a problem hiding this comment.
This isn't actually changing locations: its name should describe what it actually does.
test/test_conversion.py
Outdated
| if isinstance(o, sbol3.Location): | ||
| if hasattr(o, 'orientation') and o.orientation != None: | ||
| location_ids_sbol3.append(o.identity) | ||
| doc3.traverse(change_if_location) |
There was a problem hiding this comment.
How many should there be? Add a check to see the number.
|
Should I rewrite whole |
|
I would suggest adding another function to test conversion of orientation. The reason I suggest that is that one often gets better coverage and simpler debugging when there are a larger number of more focused tests, rather than a smaller number of more complex tests. |
|
Hi @jakebeal I have added separate test to validate orientation conversion using file |
|
Looks good --- nice work and thank you! |
Hi @jakebeal, tried to work on the issue #83,