Update tck tests #576
Update tck tests #576
Conversation
| """ | ||
| return foo() | ||
| """ | ||
| Then an error with GQL code fooCode should be raised with message fooMessage |
There was a problem hiding this comment.
Asserting on the complete message can be difficult (it often contains the complete query string, and that might be long and in parameterized scenarios (examples) it is different between runs). So I suggest to make it ...with message containing.
Or perhaps have syntax like:
Then an error with GQL code fooCode should be raised with message containing:
"""Blabla"""
There was a problem hiding this comment.
Or you could have:
Then an error with GQL code fooCode should be raised with message matching: /regex/
The question is what is actually needed. In the oC scenarios, I would advocate for just checking the code, as the message is something vendor specific.
If we want to check the precise message for the product, than I am not opposing these means. But maybe there is another add this so that the scenarios do not have to be duplicated.
Maybe the TCK API can be given a mapping of code to message regex (by the Neo4j TCK adapter, in our case). When evaluating the error step of a scenario the TCK API checks is it has a mapping for the code. If it has none, it does not check the message. If it has one, it test the message too. How about that?
This would allow us to test messages for consistency without unnecessarily detailing the oC TCK with messages.
There was a problem hiding this comment.
This part sounds like a great ideia. Considering GQL codes have a 1 to 1 mapping to messages (which are parameterized).
There was a problem hiding this comment.
This goes in the right direction. I have left a fee comments.
The two main things are
- I think we can better separate our the old stuff.
- There is a bunch of code in tools, that need adjustment, too. Primarily the integrity tests, cf. tools/tck-integrity-tests/src/test/scala/org/opencypher/tools/tck/ValidateSteps.scala — if you rename ExpectError, what will give you a hint where adjustments are needed.
tools/tck-api/src/main/scala/org/opencypher/tools/tck/api/Scenario.scala
Outdated
Show resolved
Hide resolved
| """ | ||
| return foo() | ||
| """ | ||
| Then an error with GQL code fooCode should be raised with message fooMessage |
There was a problem hiding this comment.
Or you could have:
Then an error with GQL code fooCode should be raised with message matching: /regex/
The question is what is actually needed. In the oC scenarios, I would advocate for just checking the code, as the message is something vendor specific.
If we want to check the precise message for the product, than I am not opposing these means. But maybe there is another add this so that the scenarios do not have to be duplicated.
Maybe the TCK API can be given a mapping of code to message regex (by the Neo4j TCK adapter, in our case). When evaluating the error step of a scenario the TCK API checks is it has a mapping for the code. If it has none, it does not check the message. If it has one, it test the message too. How about that?
This would allow us to test messages for consistency without unnecessarily detailing the oC TCK with messages.
| @@ -82,4 +82,4 @@ object CypherValueRecords { | |||
| } | |||
| } | |||
|
|
|||
| case class ExecutionFailed(errorType: String, phase: String, detail: String, exception: Option[Throwable] = None) | |||
| case class ExecutionFailed(errorType: String, gqlCode: String, message: String, phase: String, detail: String, exception: Option[Throwable] = None) | |||
There was a problem hiding this comment.
Can we split this into two classes? Something like this:
sealed trait ExecutionFailed
case class ExecutionFailedWithLegacyDetail(errorType: String, phase: String, detail: String, exception: Option[Throwable] = None) extends ExecutionFailed
case class ExecutionFailedWithGQLCode(gqlCode: String, message: String, exception: Option[Throwable] = None) extends ExecutionFailed
Maybe that is too harsh of an API change. But on the other hand, it makes clearer where this is supposed to go. The GQL code is the means to get rid of errortype, phase, and detail.
There was a problem hiding this comment.
Done! The only problem I encountered with this was that I was not sure how to decide on which of them to instantiate in certain parts of the code.
|
|
||
| case class ExpectErrorWithGQLCodeAndMessage(gqlCode: String, message: String, source: io.cucumber.core.gherkin.Step) extends Step { | ||
| } | ||
|
|
||
| case class ExpectError(errorType: String, phase: String, detail: String, source: io.cucumber.core.gherkin.Step) extends Step { |
There was a problem hiding this comment.
May be this can be renamed, too, e.g.:
| case class ExpectError(errorType: String, phase: String, detail: String, source: io.cucumber.core.gherkin.Step) extends Step { | |
| case class ExpectErrorWithLegacyDetail(errorType: String, phase: String, detail: String, source: io.cucumber.core.gherkin.Step) extends Step { |
I have a similar suggestion further down.
There was a problem hiding this comment.
Agreed, easier to differentiate which type of Errors we are dealing with.
There was a problem hiding this comment.
In order to separate ExecutionFailed instantiations had to add extra case where it checks for query containing "fooo()" instead. Feels a bit wrong.
In connection with the new GQL status standard I added support for two types of error tests:
1 - Test which relies on the GQL status code of the exception.
2 - Test which relies on both the GQL status code and the message of the exception.