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

updated logic for endNodePattern to include label information in the variable name #296

Closed
wants to merge 5 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 24, 2023

updated logic for endNodePattern to include label information in the variable name.

It fixes #295

@Andy2003
Copy link
Collaborator

Can you add a test that coveres the bug (with the wrong where-parameters)

@Andy2003 Andy2003 added the fix Bugfix label Aug 25, 2023
@ghost
Copy link
Author

ghost commented Aug 25, 2023

Added a test case to validate variable name in where condition.

@ghost
Copy link
Author

ghost commented Aug 30, 2023

Hi @Andy2003
I have added a test case to validate variable name in where condition.
Can you please review it?
Thanks

@@ -72,7 +86,24 @@ UNWIND [{
AS props

CREATE (p:Person)-[:WORKS_AT]->(c)
SET p = props
SET p = props;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why adding test data, if you don't use it in the tests?

Copy link
Author

Choose a reason for hiding this comment

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

In below tests, there are graphql queries which are filtering on node properties. So this "props" is required here.

But in my test case for "wrong variable name in where condition", props is not required. I will remove it from there.

Copy link
Author

Choose a reason for hiding this comment

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

Removed unwanted data from my test case

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I meant is that if you add test data, which is perfectly OK, then the test should also check if a particular query returns that data. This is tested using the following block:

.GraphQL-Response
[source,json,response=true,ignore-order]
----
{
 "expected": "graphql response"
}
----

Copy link
Author

Choose a reason for hiding this comment

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

Added Graphql Response

@ghost ghost requested a review from Andy2003 September 5, 2023 20:33
Copy link
Collaborator

@Andy2003 Andy2003 left a comment

Choose a reason for hiding this comment

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

Do you want to add integration test assertions?

@ghost
Copy link
Author

ghost commented Sep 8, 2023

Hi.
I will add it today. I got held up with some other work.

@ghost
Copy link
Author

ghost commented Sep 8, 2023

Do you want to add integration test assertions?

You are talking about adding this response in my test, right?

.GraphQL-Response
[source,json,response=true,ignore-order]
----
{
 "expected": "graphql response"
}
----

@Andy2003
Copy link
Collaborator

Andy2003 commented Sep 9, 2023

Please fix the tests!

 into bug-fix-wrong-variable-name-for-nested-node
@ghost
Copy link
Author

ghost commented Sep 9, 2023

Please fix the tests!

In my local, all the CypherTests are running fine.

picc

After running mvn clean install, I don't see any test failure in Neo4j GraphQL Java 1.7.1-SNAPSHOT

image

@ghost ghost closed this Sep 12, 2023
@ghost ghost deleted the bug-fix-wrong-variable-name-for-nested-node branch September 12, 2023 08:08
@Andy2003
Copy link
Collaborator

Why did you close this PR? The reason the test ware failing here is, that the integration are also ran. These tests are disabled by default, so your mvn clean install does not run these tests.

@Andy2003
Copy link
Collaborator

I will take a look at this

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong node name used for end node in rich relationship
1 participant