Add tests to sparql.rs - Githubissues

pchampin / sophia_rs

Sophia: a Rust toolkit for RDF and Linked Data

Other

210 stars 23 forks source link

Add tests to sparql.rs #92

Closed lulu-berlin closed 10 months ago

lulu-berlin commented 3 years ago

@pchampin: To help with test coverage, I added tests to the default implementation of the trait methods in sparql.rs.

This pull request targets your pull request #91 as a suggested addition.

pchampin commented 3 years ago

@yever thanks for this contribution, and sorry for the long delay. I don't think I will keep those tests, because the sparql::dummy module is really just that, a dummy module. Its purpose was to check that I could build an (fake) implementation of the proposed traits that would satisfy the compiler (the problem with generic code is that sometimes a trait compiles fine, but implementing it in a way that satisfies the compiler proves to be impossible, or very hard). So before I merge the PR, the dummy module will probably go away...

lulu-berlin commented 3 years ago

@pchampin You are welcome and of course this PR is only a suggestion.

My intention was not to test the dummy module (which is anyway flagged to be compiled only for the tests), but to test through it the implementation of SparqlResult which is part of the public API.

The methods whose implementation I am testing are:

SparqlResult::into_bindings
SparqlResult::into_boolean
SparqlResult::into_triples

If you don't wish to test them, or you wish to test them in a different way, I'll close this PR.

pchampin commented 3 years ago

Ok, I missed that, sorry. So yes, it makes sense to test those. But the fact is that you are relying on the dummy MyDataset implementation of SparqlDataset... If you don't mint, I suggest you wait until #91 is cleaned and merged, and then we can see how to proper test it. Hopefully, I'll attend to that quickly (just waiting for your feedback and Tpt's on my last commit).

Thanks again.

lulu-berlin commented 3 years ago

Ok. I converted this pull request into a draft. I'll create a new one after #91 is merged.

I'll try to come up with a way to test without relying on MyDataset.

pchampin commented 3 years ago

Actually, the implementation of SparqlBindings was also part of the dummy module, so I removed it when merging the PR. I am not sure any type is an "obvious" candidate for being a default generic implementation for this trait...