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.

Sign up for GitHub

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

Jump to bottom

[incubator-kie-drools-5792] [new-parser] improve drools-drl-parser-tests #5855

Merged
merged 3 commits into from
Apr 23, 2024
Merged

[incubator-kie-drools-5792] [new-parser] improve drools-drl-parser-tests #5855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e75d0d6
[incubator-kie-drools-5854] [new-parser] improve drools-drl-parser-te…
tkobayas Apr 19, 2024
fa142c3
Update drools-drl/drools-drl-parser/src/main/java/org/drools/drl/pars…
tkobayas Apr 23, 2024
93f38d9
- removed unused import
tkobayas Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions drools-drl/drools-drl-parser-tests/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,42 @@
</dependency>
</dependencies>

<profiles>
<profile>
<id>default</id>
<activation>
<activeByDefault>true</activeByDefault>
</activation>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<executions>
<execution>
<!-- override default-test for new parser -->
<id>default-test</id>
<configuration>
<systemPropertyVariables>
<drools.drl.antlr4.parser.enabled>true</drools.drl.antlr4.parser.enabled>
</systemPropertyVariables>
</configuration>
</execution>
<execution>
<id>old-parser-test</id>
<configuration>
<systemPropertyVariables>
<drools.drl.antlr4.parser.enabled>false</drools.drl.antlr4.parser.enabled>
</systemPropertyVariables>
</configuration>
<goals>
<goal>test</goal>
</goals>
</execution>
Comment on lines +98 to +117
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we can run old and new parser for the drools-drl-parser-tests

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if I understand. Does this run the new tests (that we keep on adding to on this feature branch) against both the new and the old parser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yurloc Yes, in drools-drl-parser-tests, surefire runs twice with drools.drl.antlr4.parser.enabled = true and false. You would see:

[INFO] --- surefire:3.1.2:test (default-test) @ drools-drl-parser-tests ---
[INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider
[INFO] 
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.drools.drl.parser.antlr4.DRLExprParserTest
...
### parse : ANTLR4_PARSER_ENABLED = true
### parse : ANTLR4_PARSER_ENABLED = true
...
[INFO] 
[INFO] Results:
[INFO] 
[WARNING] Tests run: 351, Failures: 0, Errors: 0, Skipped: 17
[INFO] 
[INFO] 
[INFO] --- surefire:3.1.2:test (old-parser-test) @ drools-drl-parser-tests ---
[INFO] Using auto detected provider org.apache.maven.surefire.junitplatform.JUnitPlatformProvider
[INFO] 
[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.drools.drl.parser.antlr4.DRLExprParserTest
...
### parse : ANTLR4_PARSER_ENABLED = false
### parse : ANTLR4_PARSER_ENABLED = false
...
[INFO] 
[INFO] Results:
[INFO] 
[WARNING] Tests run: 351, Failures: 0, Errors: 0, Skipped: 17

</executions>
</plugin>
</plugins>
</build>
</profile>
</profiles>
</project>
91 changes: 64 additions & 27 deletions ...drools-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DRLExprParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
import org.drools.drl.ast.descr.ConstraintConnectiveDescr;
import org.drools.drl.ast.descr.RelationalExprDescr;
import org.drools.drl.parser.DrlExprParser;
import org.drools.drl.parser.DrlExprParserFactory;
import org.drools.drl.parser.DrlParser;
import org.drools.drl.parser.DroolsParserException;
import org.drools.drl.parser.impl.Operator;
import org.junit.jupiter.api.AfterEach;
Expand All @@ -49,17 +51,17 @@ public class DRLExprParserTest {
DrlExprParser parser;

@BeforeEach
public void setUp() throws Exception {
this.parser = new Drl6ExprParserAntlr4(LanguageLevelOption.DRL6);
public void setUp() {
this.parser = DrlExprParserFactory.getDrlExprParser(LanguageLevelOption.DRL6);
}

@AfterEach
public void tearDown() throws Exception {
public void tearDown() {
this.parser = null;
}

@Test
public void testSimpleExpression() throws Exception {
public void testSimpleExpression() {
String source = "a > b";
ConstraintConnectiveDescr result = parser.parse( source );
assertThat(parser.hasErrors()).as(parser.getErrors().toString()).isFalse();
Expand Down Expand Up @@ -387,13 +389,26 @@ public void testMismatchedInput() {
assertThat(parser.hasErrors()).isTrue();
assertThat(parser.getErrors()).hasSize(1);
DroolsParserException exception = parser.getErrors().get(0);
assertThat(exception.getErrorCode()).isEqualTo("ERR 102");
assertThat(exception.getLineNumber()).isEqualTo(1);
assertThat(exception.getColumn()).isEqualTo(1);
assertThat(exception.getOffset()).isEqualTo(1);
assertThat(exception.getMessage())
.startsWithIgnoringCase("[ERR 102] Line 1:1 mismatched input '<EOF>' expecting ")
.contains("TIME_INTERVAL", "DRL_STRING_LITERAL", "?/", "boolean", "byte", "char", "double", "float", "int", "long", "new", "short", "super", "DECIMAL_LITERAL", "HEX_LITERAL", "FLOAT_LITERAL", "BOOL_LITERAL", "STRING_LITERAL", "null", "(", "[", ".", "<", "!", "~", "++", "--", "+", "-", "*", "/", "IDENTIFIER");

// Backward Compatibility Notes:
// Antlr4 gives a different error code/message from antlr3 for this case.
// Backward compatibility doesn't seem to be required in this case.
Comment on lines +393 to +395
Copy link
Contributor Author

@tkobayas tkobayas Apr 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I leave Backward Compatibility Notes when test results are different between old and new parser. We can categorize the cases into:

A) The difference comes from Antrl3 and 4, so we cannot change.
B) Old parser seems to be wrong and if we mimic the old behaviour, it would get unnecessarily complex. -> but, of course, we can discuss and look for compromise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please give us one or more examples of B)? In which cases the old parser is wrong? This would help us to focus this discussion and decide if it makes sense to pollute the new parser to accommodate the weird behaviors of the old one (I hope not).

Copy link
Contributor Author

@tkobayas tkobayas Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@mariofusco Sure, commented below. Probably it's easier to look in https://github.com/apache/incubator-kie-drools/pull/5855/files

if (DrlParser.ANTLR4_PARSER_ENABLED) {
assertThat(exception.getErrorCode()).isEqualTo("ERR 102");
assertThat(exception.getLineNumber()).isEqualTo(1);
assertThat(exception.getColumn()).isEqualTo(1);
assertThat(exception.getOffset()).isEqualTo(1);
assertThat(exception.getMessage())
.startsWithIgnoringCase("[ERR 102] Line 1:1 mismatched input '<EOF>' expecting ")
.contains("TIME_INTERVAL", "DRL_STRING_LITERAL", "?/", "boolean", "byte", "char", "double", "float", "int", "long", "new", "short", "super", "DECIMAL_LITERAL", "HEX_LITERAL", "FLOAT_LITERAL", "BOOL_LITERAL", "STRING_LITERAL", "null", "(", "[", ".", "<", "!", "~", "++", "--", "+", "-", "*", "/", "IDENTIFIER");
} else {
assertThat(exception.getErrorCode()).isEqualTo("ERR 101");
assertThat(exception.getLineNumber()).isEqualTo(1);
assertThat(exception.getColumn()).isEqualTo(1);
assertThat(exception.getOffset()).isEqualTo(1);
assertThat(exception.getMessage())
.isEqualToIgnoringCase("[ERR 101] Line 1:1 no viable alternative at input '<eof>'");
}
}

@Test
Expand All @@ -403,26 +418,48 @@ public void testExtraneousInput() {
assertThat(parser.hasErrors()).isTrue();
assertThat(parser.getErrors()).hasSize(1);
DroolsParserException exception = parser.getErrors().get(0);
assertThat(exception.getErrorCode()).isEqualTo("ERR 109");
assertThat(exception.getLineNumber()).isEqualTo(1);
assertThat(exception.getColumn()).isEqualTo(3);
assertThat(exception.getOffset()).isEqualTo(3);
assertThat(exception.getMessage())
.startsWithIgnoringCase("[ERR 109] Line 1:3 extraneous input ';' expecting ")
.contains("TIME_INTERVAL", "DRL_STRING_LITERAL", "?/", "boolean", "byte", "char", "double", "float", "int", "long", "new", "short", "super", "DECIMAL_LITERAL", "HEX_LITERAL", "FLOAT_LITERAL", "BOOL_LITERAL", "STRING_LITERAL", "null", "(", "[", ".", "<", "!", "~", "++", "--", "+", "-", "*", "/", "IDENTIFIER");

// Backward Compatibility Notes:
// Antlr4 gives a different error code/message from antlr3 for this case.
// Backward compatibility doesn't seem to be required in this case.
if (DrlParser.ANTLR4_PARSER_ENABLED) {
assertThat(exception.getErrorCode()).isEqualTo("ERR 109");
assertThat(exception.getLineNumber()).isEqualTo(1);
assertThat(exception.getColumn()).isEqualTo(3);
assertThat(exception.getOffset()).isEqualTo(3);
assertThat(exception.getMessage())
.startsWithIgnoringCase("[ERR 109] Line 1:3 extraneous input ';' expecting ")
.contains("TIME_INTERVAL", "DRL_STRING_LITERAL", "?/", "boolean", "byte", "char", "double", "float", "int", "long", "new", "short", "super", "DECIMAL_LITERAL", "HEX_LITERAL", "FLOAT_LITERAL", "BOOL_LITERAL", "STRING_LITERAL", "null", "(", "[", ".", "<", "!", "~", "++", "--", "+", "-", "*", "/", "IDENTIFIER");
} else {
assertThat(exception.getErrorCode()).isEqualTo("ERR 101");
assertThat(exception.getLineNumber()).isEqualTo(1);
assertThat(exception.getColumn()).isEqualTo(3);
assertThat(exception.getOffset()).isEqualTo(3);
assertThat(exception.getMessage())
.isEqualToIgnoringCase("[ERR 101] Line 1:3 no viable alternative at input ';'");
}
}

@Test
public void testNoViableAlt() {
String source = "x.int";
parser.parse(source);
assertThat(parser.hasErrors()).isTrue();
assertThat(parser.getErrors()).hasSize(1);
DroolsParserException exception = parser.getErrors().get(0);
assertThat(exception.getErrorCode()).isEqualTo("ERR 101");
assertThat(exception.getLineNumber()).isEqualTo(1);
assertThat(exception.getColumn()).isEqualTo(2);
assertThat(exception.getOffset()).isEqualTo(2);
assertThat(exception.getMessage())
.isEqualToIgnoringCase("[ERR 101] Line 1:2 no viable alternative at input '.int'");

// Backward Compatibility Notes:
// Old expr parser (DRL6Expressions) allows this expression because it's too tolerant (fail at runtime anyway).
// Backward compatibility doesn't seem to be required in this case. (But we may align with the old tolerant behavior.)
if (DrlParser.ANTLR4_PARSER_ENABLED) {
Copy link
Contributor Author

@tkobayas tkobayas Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Backward Compatibility Notes : B) Old parser seems to be wrong

Old parser allows x.int.

assertThat(parser.hasErrors()).isTrue();
assertThat(parser.getErrors()).hasSize(1);
DroolsParserException exception = parser.getErrors().get(0);
assertThat(exception.getErrorCode()).isEqualTo("ERR 101");
assertThat(exception.getLineNumber()).isEqualTo(1);
assertThat(exception.getColumn()).isEqualTo(2);
assertThat(exception.getOffset()).isEqualTo(2);
assertThat(exception.getMessage())
.isEqualToIgnoringCase("[ERR 101] Line 1:2 no viable alternative at input '.int'");
} else {
assertThat(parser.hasErrors()).isFalse();
}
}
}
3 changes: 3 additions & 0 deletions ...drl/drools-drl-parser-tests/src/test/java/org/drools/drl/parser/antlr4/DRLParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
import static org.drools.drl.parser.antlr4.DRLParserHelper.createDrlParser;
import static org.drools.drl.parser.antlr4.DRLParserHelper.parse;

/**
* This test class is specific to new antlr4 parser.
*/
class DRLParserTest {

private static final String drl =
Expand Down
Loading
Loading