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

Convert translate_algebra tests to pytest #1636

Merged

Conversation

aucampia
Copy link
Member

@aucampia aucampia commented Dec 31, 2021

As far as I can tell all the tests inside test/translate_algebra
basically did the following:

query_tree = parser.parseQuery(self.query_text)
query_algebra = algebra.translateQuery(query_tree)
self.query_from_algebra = translateAlgebra(query_algebra)

query_tree_2 = parser.parseQuery(self.query_from_algebra)
query_algebra_2 = algebra.translateQuery(query_tree_2)
self.query_from_query_from_algebra = translateAlgebra(query_algebra_2)
_pprint_query(self.query_from_query_from_algebra)

One test tried to also execute the query, but against a None valued
variable. And then there was some exception handling in specific place.

This change converts all of this to pytest using parameterization.

There are two tests that fail, and seem to also fail in the old test
structure, they are:

AlgebraTest(
    "test_other__service1",
    "Test if a nested service pattern is properly translated"
    "into the query text.",
    pytest.mark.xfail(raises=RecursionError),
),
AlgebraTest(
    "test_property_path__negated_property_set",
    "Test if a negated property set gets properly translated into the query text.",
    pytest.mark.xfail(raises=TypeError, reason="fails in translateAlgebra"),
),

The test_other__service1 causes some more serious issues on Windows so
it is guarded in a condition to only run it on non Windows operating
systems.

Fixes #1451

@aucampia
Copy link
Member Author

@GreenfishK would appreciate it if you could have a look at this.

@aucampia aucampia force-pushed the iwana-20211230T2304-algebra_in_pytest branch 2 times, most recently from d60644e to 1c9cb42 Compare December 31, 2021 00:55
As far as I can tell all the tests inside test/translate_algebra
basically did the following:

```python
query_tree = parser.parseQuery(self.query_text)
query_algebra = algebra.translateQuery(query_tree)
self.query_from_algebra = translateAlgebra(query_algebra)

query_tree_2 = parser.parseQuery(self.query_from_algebra)
query_algebra_2 = algebra.translateQuery(query_tree_2)
self.query_from_query_from_algebra = translateAlgebra(query_algebra_2)
_pprint_query(self.query_from_query_from_algebra)
```

One test tried to also execute the query, but against a None valued
variable. And then there was some exception handling in specific place.

This change converts all of this to pytest using parameterization.

There are two tests that fail, and seem to also fail in the old test
structure, they are:

```python
AlgebraTest(
    "test_other__service1",
    "Test if a nested service pattern is properly translated"
    "into the query text.",
    pytest.mark.xfail(raises=RecursionError),
),
AlgebraTest(
    "test_property_path__negated_property_set",
    "Test if a negated property set gets properly translated into the query text.",
    pytest.mark.xfail(raises=TypeError, reason="fails in translateAlgebra"),
),
```

The `test_other__service1` causes some more serious issues on Windows so
it is guarded in a condition to only run it on non Windows operating
systems.
@aucampia aucampia force-pushed the iwana-20211230T2304-algebra_in_pytest branch from 1c9cb42 to 203bcc2 Compare December 31, 2021 01:02
@GreenfishK
Copy link

@aucampia First of all, thank's so much for your effort and sorry that I still did not find time. If there is still something I can do, I will take time for it this weekend.
Tests: I had two ideas. The first one was to roundtrip the query as you understood correctly and to compare the normalized original query with the roundtripped normalized original query. the second one was to execute the query pairs against a currated graph and to see if they both deliver the same result. At the time of writing I did not have time to set up queries for an actual currated graph so I constructed artificial ones. That is why you found one query with such an attempt, which I thought I actually removed.

@aucampia aucampia force-pushed the iwana-20211230T2304-algebra_in_pytest branch from df014ac to 052e88b Compare December 31, 2021 19:32
@aucampia
Copy link
Member Author

Tests: I had two ideas. The first one was to roundtrip the query as you understood correctly and to compare the normalized original query with the roundtripped normalized original query. the second one was to execute the query pairs against a currated graph and to see if they both deliver the same result. At the time of writing I did not have time to set up queries for an actual currated graph so I constructed artificial ones. That is why you found one query with such an attempt, which I thought I actually removed.

Thanks for the information and clarification, I added a comment capturing this intent now.

- Rename the test data directory for `translate_algebra` to match other
  test data directories. (i.e. `test_translate_algebra` -> `translate_algebra`)
- Remove `types-tabulate` form requirements.dev.txt as this was only
  used in the custom test framework.
- Fix translate algebra docstring.
- Add a TODO comment expressing the intent to evaluate algebra tests
  against a well defined graph and verify the result from the
  reconstituted query matches the result from the raw query.
@aucampia aucampia force-pushed the iwana-20211230T2304-algebra_in_pytest branch from 052e88b to f331ba9 Compare December 31, 2021 19:37
@nicholascar
Copy link
Member

@GreenfishK @aucampia is ready to be merged now, as is, but then later PRs might add the curated graph tests?

@aucampia
Copy link
Member Author

aucampia commented Jan 2, 2022

@GreenfishK @aucampia is ready to be merged now, as is, but then later PRs might add the curated graph tests?

Yes, as the tests being replaced did not have these anyway. They will technically be testing something outside the scope of algebra to query translation though, so while they may be useful they are not critical and the tests here should be sufficient for testing translateAlgebra given translateQuery is working correctly.

@nicholascar nicholascar self-requested a review January 2, 2022 13:34
Copy link
Member

@nicholascar nicholascar left a comment

Choose a reason for hiding this comment

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

Approval from me but waiting on a last word from @GreenfishK before merge

@GreenfishK
Copy link

@nicholascar @aucampia I checked the commit and it looks good to me, also the code is conciser now. Thank you!
You once asked me to add a description to https://rdflib.readthedocs.io/en/stable/intro_to_sparql.html. I plan to do that now if it is still needed.

@nicholascar
Copy link
Member

Thanks for the confirmation @GreenfishK, merging now.

@GreenfishK: it would be best to add the tests for #1361 so we can clear away all test passing PRs but they yes, love to have more material for the Intro to SPARQL! I keep promising myself to sit down and to a comprehensive set of additions to documentation before each release but I'm only making progress bit-by-bit there!

@nicholascar nicholascar merged commit 6300d58 into RDFLib:master Jan 2, 2022
@GreenfishK
Copy link

@nicholascar I am working on the tests for #1361

@aucampia aucampia deleted the iwana-20211230T2304-algebra_in_pytest branch January 8, 2022 10:13
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.

Tests for translateAlgebra are not integrated with other unit tests
3 participants