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

Add some tests covering regex quantifiers and options #155

Merged
merged 1 commit into from
Dec 5, 2024
Merged

Add some tests covering regex quantifiers and options #155

merged 1 commit into from
Dec 5, 2024

Conversation

Tpt
Copy link
Contributor

@Tpt Tpt commented Nov 25, 2024

@rubensworks
Copy link
Member

@Tpt Is there a particular reason this was only added for sparql10 and not sparql11?

@Tpt
Copy link
Contributor Author

Tpt commented Nov 26, 2024

@Tpt Is there a particular reason this was only added for sparql10 and not sparql11?

@rubensworks If I am not mistaken the sparql11 folder only contains tests for features added by SPARQL 1.1. For example BGP and basic operators (=, <, +...) tests are only in the sparql10 directory. Because REGEX was introduced in SPARQL 1.0 I added the tests there for consistency.

@rubensworks
Copy link
Member

Oh, in that case I misunderstood how the tests are structured...

@jitsedesmet
Copy link

jitsedesmet commented Nov 29, 2024

I noticed 2 more things when implementing the tests in Comunica.

First:
The result file regex-no-metacharacters-case-insensitive.srx is missing?

Second:

The following manifest entry sticks out

:regex-data-quantifier-optional a mf:QueryEvaluationTest ;
      mf:name    "REGEX with an ? quantifier" ;
      mf:action
          [ qt:query  <regex-quantifier-optional.rq> ;
            qt:data   <regex-data-quantifiers.ttl> ] ;
      mf:result  <regex-quantifier-optional.srx> .

The subject is :regex**-data-**quantifier-optional while it operates over the files regex-quantifier-optional. Is there a need for the mention of -data-?

Anyway, thank you for addressing my previous remarks :)

@Tpt
Copy link
Contributor Author

Tpt commented Nov 29, 2024

The result file regex-no-metacharacters-case-insensitive.srx is missing?

Yes, out of laziness because it should return the same results as regex-no-metacharacters.srx

The subject is :regex**-data-**quantifier-optional while it operates over the files regex-quantifier-optional. Is there a need for the mention of -data-?

None, it is an artifact or an older naming I removed late in the MR writing. Fixed.

Anyway, thank you for addressing my previous remarks :)

With pleasure! Thank you for testing this MR.

@afs
Copy link
Contributor

afs commented Dec 5, 2024

@Tpt -- Do you want to squash the commits down to one? Or I can do in the GH interface? Or leave it as 8 commits for "rebase and merge" (this repo normally uses "rebase and merge").

@Tpt
Copy link
Contributor Author

Tpt commented Dec 5, 2024

@afs Done! Buf feel free to do it in GitHub interface the next times if you prefer.

@afs afs merged commit dfeb021 into main Dec 5, 2024
1 check passed
@afs
Copy link
Contributor

afs commented Dec 5, 2024

Oh, in that case I misunderstood how the tests are structured...

@rubensworks - I don't think we have a definite policy.

More tests is better. Structure can change.

My (mild) pref would be new test areas under sparql-current. In this case, there is already existing test area and splitting them across sparql-N seems less helpful.

It does mean we are not keeping the compliance tests for Sparql-VER exectly as used at the time as implementation reports but that could be quite a burden for not much value IMO. It's a tradeoff of more tests for sparql-current over a historical record of implementation reports.

@Tpt Tpt deleted the regex branch December 20, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants