-
Notifications
You must be signed in to change notification settings - Fork 351
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
Support new Neo4j types (Neo4j 3.4) #735
Conversation
I'm leaving this PR as it is and will create a task to display types of all returned objects |
ac1dcc8
to
2238de3
Compare
2238de3
to
10b4443
Compare
- Format new type `Point` correctly and add e2e test. - Format Number that are not neo4j ints as float (always ass `.0` at the end to signal that. - Update tests that failed because of number formatting changes. Use new formatting function in both table view and ascii view
Formatting of types in output tables are now added and added screenshots to PR description. |
it('presents the point type correctly', () => { | ||
cy.executeCommand(':clear') | ||
const query = | ||
"WITH point({{}crs: 'wgs-84', longitude: 12.78, latitude: 56.7}) as p1 RETURN p1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the {}
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Escape syntax for {
. Test framework specific.
it('presents the point type correctly', () => { | ||
cy.executeCommand(':clear') | ||
const query = | ||
"WITH point({{}crs: 'wgs-84', longitude: 12.78, latitude: 56.7}) as p1 RETURN p1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't point({{}crs
be point({crs
(without additional {}
). I also see the tests pass with this approach.
Am I missing something as to why this passes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try it :)
When you "type" things in cypress we have to escape "{" because it's a special character. And this is how you escape it.
In the tests you see that it types what we want it to type.
e2e_tests/integration/types.spec.js
Outdated
it('presents temporal types correctly', () => { | ||
cy.executeCommand(':clear') | ||
const query = | ||
'RETURN datetime({{}year:2015, month:7, day:20, hour:15, minute:11, second:42, timezone:"Europe/Stockholm"}) AS t1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional {}
Create convenience method `cy.resultContains `
cy.get(SubmitQueryButton).click() | ||
}) | ||
Cypress.Commands.add('waitForCommandResult', () => { | ||
cy | ||
.get('[data-test-id="frame-loaded-contents"]', { timeout: 40000 }) | ||
.should('be.visible') | ||
}) | ||
Cypress.Commands.add('resultContains', str => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Support new Neo4j types
This PR adds support for the new Neo4j types:
This also removes the driver mock we used to have. It's getting tiresome to keep up the interface changes. No need to mock.
It formats
point
types as seen in the screenshots. Temporal types are.toString()
formatted.E2E tests are added (couldn't add unit tests because detached driver didn't output something testable).