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.

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

Use shared test data #9

Merged
merged 17 commits into from
Oct 18, 2021
Merged

Use shared test data #9

merged 17 commits into from
Oct 18, 2021

Conversation

aslakhellesoy
Copy link
Contributor

@aslakhellesoy aslakhellesoy commented Oct 11, 2021

Description

Use shared test data

  • Go
  • Java
  • JavaScript
  • Perl
  • Python
  • Ruby

Motivation & context

So we have a single source of truth for all implementations

Type of change

  • Refactoring/debt (improvement to code design or tooling without changing behaviour)

@aslakhellesoy
Copy link
Contributor Author

@ehuelsmann @jenisys would you be able to push some more commits to this branch with similar changes for Perl and Python? That way we can ensure that all implementations behave consistently, and we don't have to maintain the acceptance tests in more than one place.

Thanks!
Aslak

@ehuelsmann
Copy link
Contributor

@aslakhellesoy no problem. I'll have to look at it over the weekend. I love the idea of a single test suite, also because that ensures consistent behaviour across implementations. The perl "stringifier" can't match the output that's in the parser.yml tests at the moment: AND expressions always have exactly 2 terms (using a nested term-of-two-terms to model a 3-term AND). But I'll start with the behaviour that I can test and work my way through this from there.

@@ -0,0 +1,59 @@
- expression: 'not x'
Copy link
Contributor

Choose a reason for hiding this comment

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

@aslakhellesoy I spent some time working on this branch. I'm running into the assumption in the Perl code that tags start with an '@' sign, but in these tests, they don't. Should I remove the check for the '@'-sign to distinguish junk from tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t think the parser should require operands to start with an @. The library is a general-purpose parser/evaluator for boolean infix expressions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I'll change the Perl version of the library.

One follow-up question: I noticed you don't have a test for the following expressions:

a and or

is the or on that line an operator or and operand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ehuelsmann I added a test for that in ef521d9

The or token is an operator and should cause a parse error here.

result: false
- variables: ['z']
result: false
- expression: '\x or y\ or z\'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this expression not generate a syntax error? I'm reading these tokens in this line:

- 'x'
- or
- 'y or'
- 'z'

I'm also reading an escaping character at the end of the string, meaning there's a missing escaped character. Are the semantics of an escaped "end of line" just an end of line? (so, ignore the backslash?)

Copy link
Contributor Author

@aslakhellesoy aslakhellesoy Oct 18, 2021

Choose a reason for hiding this comment

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

I think a \ character that isn't followed by either \, ' ', ( or ) should result in a syntax error. If we agree on that, we need to change the test data and update the other parsers accordingly.

WDYT @yusuke-noda - you contributed some nice escaping fixes in cucumber/common#1778 (before we moved the library to this repo) and this seems related.

Copy link
Contributor

Choose a reason for hiding this comment

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

First, the current java, javascript, go, and ruby implementation differs from the perl implementation in the following points,
so I think that the java, javascript, go, and ruby implementation should be fixed.

  • Perl implementation can escape spaces, but other implementations cannot.
  • Perl implementation throws error when the end of the expression is an odd number of backslashes, but other implementations don't throw error.

And,

I think a \ character that isn't followed by either \, ' ', ( or ) should result in a syntax error. If we agree on that, we need to change the test data and update the other parsers accordingly.

WDYT @yusuke-noda - you contributed some nice escaping fixes in cucumber/common#1782 (before we moved the library to this repo) and this seems related.

I think its good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merged #17 into this PR, and java/ruby/javascript/go now throw errors on illegal escapes. Test data has been updated too.

@aslakhellesoy
Copy link
Contributor Author

@jenisys any update on this? The other implementations have some recent changes in how parenthesis and whitespace is escaped, and this is reflected in the shared test cases in this PR. The Python implementation has not been updated to reflect this.

We may need to disable the the build/release of the python implementation until this is fixed.

@jenisys
Copy link
Contributor

jenisys commented Oct 18, 2021

@aslakhellesoy
I will look into it in the next few days.
First I fix the CI build problem related to PR #15.

aslakhellesoy and others added 5 commits October 18, 2021 16:54
* Throw error on illegal escapes

* Fix JavaScript tests, simplyfy Java

* Update Ruby parser

* Document escaping. Fixes #16.

* Fix go escape validation (simplify java)
@aslakhellesoy aslakhellesoy marked this pull request as ready for review October 18, 2021 17:35
@aslakhellesoy aslakhellesoy merged commit 2bc84ee into main Oct 18, 2021
@aslakhellesoy aslakhellesoy deleted the shared-testdata branch October 18, 2021 17:39
@aslakhellesoy
Copy link
Contributor Author

See #18 and #19 for Python / Perl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants