Skip to content

Fix 'path' argument. Add more visitor tests #1149

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 15, 2017
Merged

Fix 'path' argument. Add more visitor tests #1149

merged 1 commit into from
Dec 15, 2017

Conversation

IvanGoncharov
Copy link
Member

@IvanGoncharov IvanGoncharov commented Dec 15, 2017 •
edited
Loading

As discussed in #1145 I started to work on tests for visit function.

@IvanGoncharov
Copy link
Member Author

IvanGoncharov commented Dec 15, 2017 •
edited
Loading

@leebyron I added the simple test for path parameter and discover that path value differs between enter and leave callbacks. On leave path is always one element shorter, for example, if you were called on entering to OperationDefinition node path value would be ['definitions', 0] but on leave, you will get only ['definitions'].

Is it intentional behavior or a bug?

mjmahone
mjmahone reviewed Dec 15, 2017
View reviewed changes
src/language/__tests__/visitor-test.js Outdated
},
});

console.log(JSON.stringify(visited, null, 2));
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably don't want to keep this console.log in the test

Copy link
Member Author

Choose a reason for hiding this comment

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

@mjmahone Consequences of committing at 3 AM 😄
Removed.

src/language/__tests__/visitor-test.js Outdated
@@ -17,6 +17,35 @@ import { testSchema } from '../../validation/__tests__/harness';
import { getNamedType, isCompositeType } from '../../type';

describe('Visitor', () => {
it('check path argument', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 'validates path argument' or 'checks path argument'

Copy link
Member Author

Choose a reason for hiding this comment

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

@mjmahone Thanks 👍 Fixed now.

@IvanGoncharov
Copy link
Member Author

IvanGoncharov commented Dec 15, 2017 •
edited
Loading

I fixed path issue with a very small fix that's easy to verify.

As for testing visitor callback arguments, I decided it's too much to add to hardcoded snapshots inside tests. So I added checkVisitorFnArgs utility function that checks types and interdependencies of all callback arguments and instrument every callback inside visitor-tests.js with it.

One thing to note is the current implementation of visit doesn't maintain consistency of arguments after AST is edited. For example, if you edit some node on enter you would get updated node on leave but parent and all ancestors will remain not updated. To workaround this problem I added isEdited argument that disables some of the checks inside checkVisitorFnArgs function.

@mjmahone @leebyron Can you please review this PR?

@IvanGoncharov IvanGoncharov changed the title [WIP]Add visitor tests Fix 'path' argument. Add more visitor tests Dec 15, 2017
@leebyron leebyron merged commit c4e301d into graphql:master Dec 15, 2017
@leebyron
Copy link
Contributor

Nice work, @IvanGoncharov

@IvanGoncharov IvanGoncharov deleted the visitorTests branch January 10, 2018 21:38
@danez danez mentioned this pull request Feb 16, 2018
Merged
45 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leebyron leebyron Awaiting requested review from leebyron

@mjmahone mjmahone Awaiting requested review from mjmahone

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@IvanGoncharov @leebyron @mjmahone @facebook-github-bot