Skip to content

Add test for LikeExpression.setEscape and LikeExpression.getStringExpression #1568

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

Merged
merged 2 commits into from
Jul 7, 2022

Conversation

lacinoire
Copy link
Contributor

Hey 😊
I want to contribute a test.
Curious to hear what you think!

(I wrote this test as part of a research study at TU Delft. Find out more)

@manticore-projects
Copy link
Contributor

What problem exactly will this PR solve and why should your test become part of the Test Suite please?

The result isn't meaningful or valid.

@lacinoire
Copy link
Contributor Author

Hey 🙂

This tests the methods LikeExpression.setEscape and LikeExpression.getStringExpression, which are not covered by another test as far as I can see.

It is based on / inspired by the test testLikeNotIssue660.

Would it be better to include a more meaninigful query instead of the 'default' null LIKE null ESCAPE null LIKE null ?

Do you find it relevant to test LikeExpression.setEscape and .getStringExpression at all? 🙂

@manticore-projects
Copy link
Contributor

Adding test coverage is definitely good -- when the tests are meaningful and assert something. Test without purpose are considered harmful, since they pretend coverage where no insurance is provided.

Better craft an actual valid Expression using the LIKE clause in combination with an ESCAPE term -- instead of NULLS.

@lacinoire
Copy link
Contributor Author

Thank you for your input and patience 🙂

I've changed the expression to match all records with names that start with letter ’J’ and have the ’_’ character in them:
"name LIKE 'J%$_%' ESCAPE '$'"
(updated the whole test case)

What do you think?

@manticore-projects
Copy link
Contributor

Thank you, looks much better now and I would like to recommend to accept your PR.
Thank you for your contribution.

@wumpz wumpz merged commit bcf6ff4 into JSQLParser:master Jul 7, 2022
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.

3 participants