-
Notifications
You must be signed in to change notification settings - Fork 23
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
Enable union validation #567
Conversation
@@ -22,7 +22,7 @@ overallSchema: | |||
# language=GraphQL | |||
underlyingSchema: | |||
Issues: | | |||
union HasName = Issue | Edible | |||
union HasName = Issue | Edible | Troll | User |
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.
Honestly not sure why these tests are set up like this… this is not desirable behavior.
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.
Shall we remove them or replace them with new tests using the new format?
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.
I think I'll just leave them, just not sure why the overall schema had more members to begin with…
@@ -474,50 +474,6 @@ class NadelTypeValidationTest : DescribeSpec({ | |||
assert(error.overallField.name == "nextgenSpecific") | |||
} | |||
|
|||
it("ignores extension union members declared by other services") { |
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.
Can we add tests for the checks performed by NadelUnionValidation
?
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.
Yeah my bad, got distracted by E2E tests and forgot. Will add some.
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.
Added
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.
Good stuff!!! Thanks for that.
Honestly not quite sure why this was commented out… running this on central schema seems fine.