search

opencypher / openCypher

Specification of the Cypher property graph query language
http://www.opencypher.org
Apache License 2.0
841 stars 149 forks source link

Stop using deprecated things in TCK tests #531

Closed Lojjs closed 2 years ago

Lojjs commented 2 years ago

The TCK tests are using syntax which have been deprecated and will be removed in Neo4j 5.0:

hvub commented 2 years ago

Looks good! One question though: What's the policy on negative tests in TCK?

There is not policy on negative test. It is a judgment call. There are necessarily infinite number of things that can make a negative test. In extreme, is "foo bar" being not a valid Cypher query worth a negative test?

I not explicit thought about this until now. I guess, a behavior could be expected to be possible or syntax combinations the arise from the language's general composition rules or grammar definition but are forbidden warrant a negative test.

I'm wondering if we should explicitly test that the replaced syntax is not accepted, as well? And for octals, possibly also that we don't accept uppercase o (0O123).

Whether my vague guideline give above applies here is a good question.

In the end, every negative test is work. If someone is willing to do the work on a negative test it provides some backing its importance.

hvub commented 2 years ago

The changes regarding octals are straightforward and perfectly good.

One more thing that jumps to mind. Have you checked the oC grammar. I guess that needs to be change accordingly.

The grammar should become something like this, I reckon:

  <production name="OctalInteger" oc:lexer="true">
    <literal value="0o"/>
    <repeat min="1"><non-terminal ref="OctDigit"/></repeat>
  </production>

If you could include that in the PR, it would be great.

Lojjs commented 2 years ago

Looks good! One question though: What's the policy on negative tests in TCK? I'm wondering if we should explicitly test that the replaced syntax is not accepted, as well? And for octals, possibly also that we don't accept uppercase o (0O123).

We might want to use a newer TCK release also for 4.x, e.g. if we find some bug in it. As the old syntax will only be removed in 5.0, I think it can be a bit cumbersome with a negative test for this case.