[incubator-kie-drools-5938] [new-parser] Tests using wrong duration a… #5969
Conversation
|
I choose the approach to meet the same approach as old parser --- use |
|
@yurloc @mariofusco @gitgabrio Please review, thanks! |
| @@ -5216,4 +5216,18 @@ void doubleAmpersandInfixAndInAccumulate() { | |||
| assertThat(accumulateFunction.getFunction()).isEqualTo("average"); | |||
| assertThat(accumulateFunction.getParams()).containsExactly("$a + $b"); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
Hi @tkobayas
IIUC a method has been removed related to this test.
Is there also a "fails" test or similar, i.e. something that demonstrates a managed excpetion, or error, when the duration input si "wrong" ?
There was a problem hiding this comment.
I'm fine with the old parser approach.
I would maybe slightly change the duration value used in the test. Provided that tests often play the role of a specification, 30s 10s might be misleading for a newcomer as it is not clear whether that represents a valid or an invalid value. It could be mistakenly interpreted as if the duration attribute accepted multiple time literals.
Since this specific test proves that any kind of input is accepted on top of valid time literals, I would maybe choose something that is more obviously wrong, for example, wrong input.
| @@ -445,7 +445,7 @@ attribute : name=( 'salience' | 'enabled' ) conditionalAttributeValue #expressio | |||
| | name=( 'agenda-group' | 'activation-group' | 'ruleflow-group' | 'date-effective' | 'date-expires' | 'dialect' ) DRL_STRING_LITERAL #stringAttribute | |||
| | name='calendars' DRL_STRING_LITERAL ( COMMA DRL_STRING_LITERAL )* #stringListAttribute | |||
| | name='timer' ( DECIMAL_LITERAL | LPAREN chunk RPAREN ) #intOrChunkAttribute | |||
| | name='duration' ( DECIMAL_LITERAL | TIME_INTERVAL | LPAREN TIME_INTERVAL RPAREN ) #durationAttribute | |||
| | name='duration' ( DECIMAL_LITERAL | LPAREN chunk RPAREN ) #intOrChunkAttribute | |||
There was a problem hiding this comment.
Hi @tkobayas IIUC a method has been removed related to this test. Is there also a "fails" test or similar, i.e. something that demonstrates a managed excpetion, or error, when the duration input si "wrong" ?
- Using the
chunkrule makes the parser extremely benevolent wrt. the duration attribute's value. The input basically cannot be wrong because thechunkrule accepts anything between the two parentheses. So no need for a negative test, IMO. - Wrt. the removed method - the method name corresponds to the rule alternative's label (string after
#). In this case, the label has changed fromdurationAttributetointOrChunkAttributemaking thevisitDurationAttribute()method unused. ThevisitIntOrChunkAttribute()method already exists (it handles for example the timer subrule one line above) so no need to add a new method.
There was a problem hiding this comment.
Thanks @yurloc , but now I'm slightly confused. IIUC, the parser does not do any kind of data validation, i.e. anything can be parsed as "DURATION", delegating the verification to the client code; and I have the impression it should not be that way (mind you, I may be wrong, of course, especially in that context)
@mariofusco wdyt ?
There was a problem hiding this comment.
Hi @gitgabrio
anything can be parsed as "DURATION", delegating the verification to the client code;
Indeed, the old Antlr3 parser delegates the verification to the compile phase, so the rule build reports an error anyway.
At the moment (= until this new parser passes all existing unit tests), I'd like to follow the same approach as the old Antlr3 parser as possible, so that we can minimize the behaviour difference now.
Then, we will consider any possible improvements. I'll note them in #5972
Does it sound good?
Hi @yurloc . Thank you for the follow-up for the Gabriele's question.
I would maybe choose something that is more obviously wrong, for example, wrong input.
Thanks, I'll do that.
There was a problem hiding this comment.
HI @tkobayas , thanks for explanation, fine for me!
apache#5969) * [incubator-kie-drools-5938] [new-parser] Tests using wrong duration attribute format * - Use explicit test value
apache#5969) * [incubator-kie-drools-5938] [new-parser] Tests using wrong duration attribute format * - Use explicit test value
apache#5969) * [incubator-kie-drools-5938] [new-parser] Tests using wrong duration attribute format * - Use explicit test value
#5969) * [incubator-kie-drools-5938] [new-parser] Tests using wrong duration attribute format * - Use explicit test value
apache#5969) * [incubator-kie-drools-5938] [new-parser] Tests using wrong duration attribute format * - Use explicit test value
…ttribute format
Issue: