Refactor the unit tests for the SPARQL ANTLR parser #767

Qup42 · 2022-08-28T19:12:08Z

The tests themselves are the same regarding content. The've just been changed to use the new parsing functions and Matchers.

Many matchers are now only combinations of other matchers. This has the benefit that the failure messages are much more usefull. Sometimes it wasn't possible to model the matcher as a combination of other matchers (e.g. IsSelect). I have started to provide manual failure messages in these cases.

…ssible

joka921

Very nice, this is a great step towards simplifying the testing and for us as a group to further understand Googletest.

When you have read all the stuff I wrote, maybe we can discuss some of it in person!

test/SparqlAntlrParserTestHelpers.h

joka921 · 2022-09-02T12:34:43Z

test/SparqlAntlrParserTestHelpers.h

@@ -263,50 +290,87 @@ MATCHER_P(GroupByVariablesMatch, vars, "") {
                    [](auto& var, auto& var1) { return var.name() == var1; });
 }

-MATCHER_P2(IsValues, vars, values, "") {
+auto IsValues = [](const vector<string>& vars,


There are some matchers above this one that are still MATCHER_P... .
Can you refactor all of them to lambdas?
As I understand it, the "base cases" all need to be builtin, non-macro matchers like testing::Field or testing::Property.

I know that this (like all of them) would incur some visual overhead, but we would be much cleaner and much easier to reason about for future refactorings.

I have currently refactored almost all Matchers to lambdas that are possible to refactor with my current knowledge. The problems are

some matchers use unwrapVariant which a custom behaviour and is not available as matcher.

std::equal in GroupByVariablesMatch but i think this one can just be a simple Eq matcher if the argument is set as vector<Variable> in the lambda

IsSelect ~~and IsSolutionModifier are~~ is very complex

IsGraphPattern should be possible to refactor with my new matcher.

test/SparqlAntlrParserTestHelpers.h

test/SparqlAntlrParserTest.cpp

Qup42 · 2022-09-02T14:49:11Z

A meeting also seems sensible to me. I was able to write a Matcher that takes variadic matchers and can then match a vector in the meantime. The matchers are tested one to one to the vectors elements. So 1st matcher on 1st vector elements, 2nd matcher on 2nd element, ... This would make it possible to express the GraphPattern Matcher as a lambda. It should also enable us to get rid of expectCompleteArrayParse which is the same pattern.

Co-authored-by: joka921 <joka921@users.noreply.github.com>

Qup42 · 2022-09-03T12:22:26Z

It seems that clang13 does not support the features required for the usage of std::source_location: llvm/llvm-project#48230

joka921

This is really coming forward.
I think we are at the point where the tests are nice when you read the test failure,
But the actual source code can be further simplified.
I think there are some aspects that we can discuss.

test/SparqlAntlrParserTest.cpp

test/SparqlAntlrParserTestHelpers.h

test/SparqlAntlrParserTest.cpp

joka921

Really nice, we are almost there.
mostly just very small stuff, and one suggestion whcih we can maybe discuss in person.

src/util/SourceLocation.h

test/SparqlAntlrParserTestHelpers.h

joka921 · 2022-09-05T18:53:26Z

test/SparqlAntlrParserTest.cpp

+  expectNumericLiteral("3.0", NumericLiteralFP(3.0));
+  expectNumericLiteral("3.0e2", NumericLiteralFP(300.0));
+  expectNumericLiteral("3.0e-2", NumericLiteralFP(0.030));
+  expectNumericLiteral("3", NumericLiteralWhole(3ll));
+  expectNumericLiteral("-3.0", NumericLiteralFP(-3.0));
+  expectNumericLiteral("-3", NumericLiteralWhole(-3ll));
+  expectNumericLiteral("+3", NumericLiteralWhole(3ll));
+  expectNumericLiteral("+3.02", NumericLiteralFP(3.02));
+  expectNumericLiteral("+3.1234e12", NumericLiteralFP(3123400000000.0));
+  expectNumericLiteral(".234", NumericLiteralFP(0.234));


I think you meant those when you wanted to pass a matcher to the ExpectCompleteParse, s.t. this can become
expectFloat("3.0", 3.0) and expectInt(+3, 3ll).
Maybe have a look on whether this can be done easily, and contact me if there's trouble.

test/SparqlParserTest.cpp

joka921 · 2022-09-06T07:25:28Z

test/SparqlAntlrParserTest.cpp

+  expectNumericLiteral("3.0", m::NumericLiteralFP(3.0));
+  expectNumericLiteral("3.0e2", m::NumericLiteralFP(300.0));
+  expectNumericLiteral("3.0e-2", m::NumericLiteralFP(0.030));
+  expectNumericLiteral("3", m::NumericLiteralWhole(3ll));
+  expectNumericLiteral("-3.0", m::NumericLiteralFP(-3.0));
+  expectNumericLiteral("-3", m::NumericLiteralWhole(-3ll));
+  expectNumericLiteral("+3", m::NumericLiteralWhole(3ll));


You were talking about making the EQ default matcher configurabel, s.t we can easily make this

auto expectFloat = exepectCompleteParse<&parseNumericLiteral, m::NumericLiteralFP>{}; expectFloat("3.0", 3.0)

Maybe we can talk about this in a meeting how it can be done, you basically need to make the Value a template that
requires that it is a valid argument to the DefaultMatcher.

Co-authored-by: joka921 <joka921@users.noreply.github.com>

Qup42 · 2022-09-06T13:41:52Z

test/SparqlAntlrParserTestHelpers.h

-    return blankNode->isGenerated() == generated && blankNode->label() == label;
+ private:
+  static std::string GetTypeName() {
+    return testing::internal::GetTypeName<ad_utility::Last<Ts...>>();


This is also an internal gtest function. But getting a type as a string in c++ seems to non trivial. testing::internal::GetTypeName<...>() does somewhat lowlevel stuff. I don't whether it is worth reimplementing the functionality to get rid of this.

joka921

Ok,
The very last and very simple suggestions before I will definitely merge this.

Please note, that we now use clang-format-14 for formatting, but this should make no difference for this code.

src/util/SourceLocation.h

test/SparqlAntlrParserTestHelpers.h

Co-authored-by: joka921 <joka921@users.noreply.github.com>

joka921

Thank you very much!
This was a lot of work, but helps a great deal for cleaner testing now, as well as in the future!

Qup42 added 2 commits August 28, 2022 20:53

Cleanup tests to provide better error messages in many cases

4d9bd4a

Replace nested matcher with lambda that constructs a matcher where po…

2735fb7

…ssible

Qup42 mentioned this pull request Aug 30, 2022

Remove unused code from SparqlParser #769

Merged

Qup42 added 3 commits August 31, 2022 00:10

Fix compile errors

0526656

Merge branch 'master' into ANTLR/testCleanup

17b2a80

Further generalize expect...

d48b937

joka921 reviewed Sep 2, 2022

View reviewed changes

Code Review

98bb35e

Qup42 and others added 6 commits September 2, 2022 17:49

Code Review

f7f7362

Update test/SparqlAntlrParserTest.cpp

d74a3fa

Co-authored-by: joka921 <joka921@users.noreply.github.com>

Fix Build

00b33ed

Code Review

fd3d9ab

Shorten code

050440a

Add source location

577eadc

Qup42 added 2 commits September 3, 2022 19:48

Explicitly specify return type

ae9ec2c

Add overloads for Matcher Lambdas

93a8f12

joka921 reviewed Sep 5, 2022

View reviewed changes

Qup42 added 6 commits September 5, 2022 11:59

Code Review

834b4bc

Code Review

89867e8

Code Review

00d22f5

Fix source_location

29d73a7

Code Review

f9206b3

Code Review

d22b53e

Qup42 marked this pull request as ready for review September 5, 2022 20:02

Rename Matchers

333765f

joka921 reviewed Sep 6, 2022

View reviewed changes

Qup42 and others added 2 commits September 6, 2022 15:14

Apply suggestions from code review

f92adcc

Co-authored-by: joka921 <joka921@users.noreply.github.com>

Code Review

15e78f1

Code Review

8869d47

Qup42 commented Sep 6, 2022

View reviewed changes

Code Review

0b17c9e

joka921 reviewed Sep 6, 2022

View reviewed changes

src/util/SourceLocation.h Outdated Show resolved Hide resolved

test/SparqlAntlrParserTestHelpers.h Outdated Show resolved Hide resolved

test/SparqlAntlrParserTestHelpers.h Outdated Show resolved Hide resolved

Qup42 and others added 2 commits September 7, 2022 11:17

Apply suggestions from code review

13bf7bb

Co-authored-by: joka921 <joka921@users.noreply.github.com>

Code Review

33ab3ea

joka921 approved these changes Sep 7, 2022

View reviewed changes

joka921 changed the title ~~Cleanup ANTLR Tests~~ Refactor the unit tests for the SPARQL ANTLR parser Sep 7, 2022

joka921 merged commit f89cedb into ad-freiburg:master Sep 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the unit tests for the SPARQL ANTLR parser #767

Refactor the unit tests for the SPARQL ANTLR parser #767

Qup42 commented Aug 28, 2022

joka921 left a comment

joka921 Sep 2, 2022

Qup42 Sep 2, 2022 •

edited by joka921

Loading

Qup42 commented Sep 2, 2022

Qup42 commented Sep 3, 2022 •

edited

Loading

joka921 left a comment

joka921 left a comment

joka921 Sep 5, 2022

joka921 Sep 6, 2022

Qup42 Sep 6, 2022

joka921 left a comment

joka921 left a comment

Refactor the unit tests for the SPARQL ANTLR parser #767

Refactor the unit tests for the SPARQL ANTLR parser #767

Conversation

Qup42 commented Aug 28, 2022

joka921 left a comment

Choose a reason for hiding this comment

joka921 Sep 2, 2022

Choose a reason for hiding this comment

Qup42 Sep 2, 2022 • edited by joka921 Loading

Choose a reason for hiding this comment

Qup42 commented Sep 2, 2022

Qup42 commented Sep 3, 2022 • edited Loading

joka921 left a comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

joka921 Sep 5, 2022

Choose a reason for hiding this comment

joka921 Sep 6, 2022

Choose a reason for hiding this comment

Qup42 Sep 6, 2022

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

joka921 left a comment

Choose a reason for hiding this comment

Qup42 Sep 2, 2022 •

edited by joka921

Loading

Qup42 commented Sep 3, 2022 •

edited

Loading