Skip to content
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

Issue 2164 #2165

Merged
merged 7 commits into from
Dec 24, 2022
Merged

Issue 2164 #2165

merged 7 commits into from
Dec 24, 2022

Conversation

hsolbrig
Copy link
Contributor

Summary of changes

Added a test case for issue #2164 and proposed a fix. The problem arose in line #415 in context.py, where the base URI passed to _prep_sources was overwritten in anticipation of processing inner nestings. Changed the assignment to base to new_base

(should have run the _whole_ test suite before submitting)
@hsolbrig
Copy link
Contributor Author

This appears to be happening because of test_localsuite.py expects the data to be located in https://rdflib.github.io/rdflib-jsonld/local-testsuite/ instead of the local testing directory. @ashleysommer - I need some advice on where to put this test. Note: I was able to pass it locally by adding the re-assignment of TC_BASE below

testsuite_dir = p.join(p.abspath(p.dirname(__file__)), "local-suite")
TC_BASE = testsuite_dir + os.sep

@coveralls
Copy link

coveralls commented Nov 22, 2022

Coverage Status

Coverage increased (+0.0005%) to 90.633% when pulling eb0fbe0 on hsolbrig:issue_2164 into 0e61a7f on RDFLib:main.

@aucampia
Copy link
Member

pre-commit.ci autofix

@aucampia aucampia requested a review from a team December 19, 2022 21:45
@aucampia
Copy link
Member

This appears to be happening because of test_localsuite.py expects the data to be located in https://rdflib.github.io/rdflib-jsonld/local-testsuite/ instead of the local testing directory.

The reason this URI is used is so that there is a system neutral way of referring to tests and test content, the best reference I can find for this now is this

The home of the test suite is <http://www.w3.org/2013/TurtleTests/>.
Per RFC 3986 section 5.1.3, the base IRI for parsing each file is the
retrieval IRI for that file. For example, the tests turtle-subm-01 and
turtle-subm-27 require relative IRI resolution against a base of
<http://www.w3.org/2013/TurtleTests/turtle-subm-01.ttl> and
<http://www.w3.org/2013/TurtleTests/turtle-subm-27.ttl> respectively.

There really are no good or simple solutions to this. We had a similar problem for the SPARQL test suite and there I resorted to monkey patching:

def patched_query_context_load(uri_mapper: URIMapper) -> Callable[..., Any]:
def _patched_load(
self: QueryContext, source: URIRef, default: bool = False, **kwargs
) -> None:
public_id = None
use_source: Union[URIRef, Path] = source
format = guess_format(use_source)
if f"{source}".startswith(("https://", "http://")):
use_source = uri_mapper.to_local_path(source)
public_id = source
if default:
assert self.graph is not None
self.graph.parse(use_source, format=format, publicID=public_id)
else:
self.dataset.parse(use_source, format=format, publicID=public_id)
return _patched_load
def check_query(monkeypatch: MonkeyPatch, entry: SPARQLEntry) -> None:
assert entry.query is not None
assert isinstance(entry.result, URIRef)
monkeypatch.setattr(
QueryContext, "load", patched_query_context_load(entry.uri_mapper)
)

In this case I think what you did is fine as there is no need for system neutral references in the local-suite JSON-LD tests.

The change and tests looks fine to me otherwise. I will just do a double check tomorrow but I think it is good to merge.

@aucampia
Copy link
Member

Okay I made a small change to instead use a file URI for TC_BASE and also added a comment to explain why, this should keep things closer to the way it was.

Copy link
Member

@aucampia aucampia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good to me @hsolbrig - I will merge by this weekend if there is no further feedback.

@aucampia aucampia requested a review from a team December 20, 2022 23:37
@aucampia aucampia added review wanted This indicates that the PR is ready for review ready to merge The PR will be merged soon if no further feedback is provided. labels Dec 21, 2022
@aucampia aucampia merged commit d74c03a into RDFLib:main Dec 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge The PR will be merged soon if no further feedback is provided. review wanted This indicates that the PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants