Skip to content
This repository has been archived by the owner on Sep 19, 2023. It is now read-only.

Copy the spec tests for aggregates #41

Closed
wschella opened this issue May 20, 2019 · 10 comments
Closed

Copy the spec tests for aggregates #41

wschella opened this issue May 20, 2019 · 10 comments
Labels
difficulty:easy This should be easy to implement or fix priority:medium test
Milestone

Comments

@wschella
Copy link
Member

As we did for the spec tests for functions, we should copy the spec tests for aggregates from the test-repo.

@wschella wschella added difficulty:easy This should be easy to implement or fix priority:medium test labels May 20, 2019
@wschella wschella added this to the Fully tested milestone May 20, 2019
@jitsedesmet
Copy link
Member

@wschella or @rubensworks, do you have a reference on where to find these test?

@rubensworks
Copy link
Member

Probably these ones: https://github.com/w3c/rdf-tests/tree/main/sparql11/data-sparql11/aggregates

@jitsedesmet
Copy link
Member

Since these are also a lot of files, I will split the spec directory.

@jitsedesmet
Copy link
Member

I've been trying for quite some time now. I have no idea on how to test this within sparqlee.
Take for example the agg2 test. If I'm not mistaken we have a whole actor in comunica to handle these kind of combinations? How would we go about testing this here?

@rubensworks
Copy link
Member

I don't think the intention was to copy the tests as-is. But instead, to select the elements of these tests that are applicable to sparqlee, i.e., those that interact directly with the aggregate evaluator.

@jitsedesmet
Copy link
Member

So we would just exclude the agg2 test? because I don't think we can test this.

@rubensworks
Copy link
Member

You'll know better than me, but can't we test the COUNT part?
If I remember correctly, grouping is done purely on the Comunica side, so that can't be tested in Sparqlee indeed.

@jitsedesmet
Copy link
Member

Yes, I guess we could. I'll write the tests. Doesn't really seem all that valuable to me but I guess it's nice to have.

@jitsedesmet
Copy link
Member

I think that this issue can be closed because of #131? Like I said here before (#41 (comment)) the test is already included in the Comunica repo.

@rubensworks
Copy link
Member

Sure.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
difficulty:easy This should be easy to implement or fix priority:medium test
Projects
None yet
Development

No branches or pull requests

3 participants