-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Test cleanup: typeResolver builder on unions and interfaces, applied directive tests, and more #2954
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
Conversation
| * @return this builder | ||
| * | ||
| * @deprecated use {@link graphql.schema.GraphQLCodeRegistry.Builder#typeResolver(GraphQLInterfaceType, TypeResolver)} instead | ||
| */ |
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.
Add doc to make the alternative clearer
| * @return this builder | ||
| * | ||
| * @deprecated use {@link graphql.schema.GraphQLCodeRegistry.Builder#typeResolver(GraphQLUnionType, TypeResolver)} instead | ||
| */ |
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.
Add doc to make the alternative clearer
| e.message.contains("an illegal value for the argument ") | ||
| } | ||
| def "Applied schema directives arguments are validated for programmatic schemas"() { |
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.
Moved to another file
| @Override | ||
| InstrumentationContext<ExecutionResult> beginExecution(InstrumentationExecutionParameters parameters) { | ||
| assert parameters.getInstrumentationState() == instrumentationState | ||
| assert parameters.getInstrumentationState() == instrumentationState // Retain for test coverage |
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.
Everything in this file is intentionally deprecated
| .type(GraphQLString)) | ||
| .build()) | ||
| .defaultValue(new FooBar()) | ||
| .defaultValue(new FooBar()) // Retain for test coverage. There is no alternative method that sets an internal value. |
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.
The alternatives are defaultValueLiteral(Value) or defaultValueProgrammatic(Object), neither of which work in this case.
|
|
||
| } | ||
|
|
||
|
|
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.
These are old tests, a typeResolver is no longer required on the union/interface type
| intDirective.name == "intDirective" | ||
| intDirective.arguments.size() == 1 | ||
| def directiveArg = intDirective.getArgument("inception") | ||
| def intAppliedDirective = argInt.getAppliedDirective("intDirective") |
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.
Key change: the applied directive contains the value, and the directive contains the default value
|
|
||
| class TypesImplementInterfacesTest extends Specification { | ||
|
|
||
| TypeResolver typeResolver = new TypeResolver() { |
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.
These are older tests, typeResolver not required directly on interface/union type
| } | ||
| } | ||
|
|
||
|
|
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 halways hated that new line
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.
Unnecessary new lines drive me
nuts
|
|
||
| class TypeAndFieldRuleTest extends Specification { | ||
|
|
||
|
|
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.
that new line has bugged me forever!
| e.message == "invalid schema:\n\"InputType\" must define one or more fields." | ||
| } | ||
|
|
||
|
|
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.
same with that one
| def e = thrown(InvalidSchemaException) | ||
| e.message.contains("Invalid argument 'arg' for applied directive of name 'cached'") | ||
| } | ||
|
|
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.
Nice one
| then: | ||
| interfaceType.getName() == "TestInterfaceType" | ||
| interfaceType.getFieldDefinition("NAME").getType() == GraphQLInt | ||
| } |
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.
nice
bbakerman
left a comment
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.
Great detective work. Getting rid of deprecation is harder than it looks
|
Dont forget to set a target milestone on PRs |
This PR fixes more tests with deprecated methods.
This is the last wave of Groovy test fixes. Next PR will fix deprecated usages in a few Java test/util files.