Skip to content

Conversation

@dondonz
Copy link
Member

@dondonz dondonz commented Sep 10, 2022

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.

* @return this builder
*
* @deprecated use {@link graphql.schema.GraphQLCodeRegistry.Builder#typeResolver(GraphQLInterfaceType, TypeResolver)} instead
*/
Copy link
Member Author

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
*/
Copy link
Member Author

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"() {
Copy link
Member Author

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
Copy link
Member Author

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.
Copy link
Member Author

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.


}


Copy link
Member Author

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")
Copy link
Member Author

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() {
Copy link
Member Author

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

}
}


Copy link
Member

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

Copy link
Member Author

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 {


Copy link
Member

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."
}


Copy link
Member

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'")
}

Copy link
Member

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
}
Copy link
Member

Choose a reason for hiding this comment

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

nice

@bbakerman bbakerman self-requested a review September 10, 2022 23:27
Copy link
Member

@bbakerman bbakerman left a 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

@bbakerman bbakerman added this to the 20.0 milestone Sep 10, 2022
@bbakerman
Copy link
Member

Dont forget to set a target milestone on PRs

@dondonz dondonz merged commit 113db6b into master Sep 11, 2022
@dondonz dondonz deleted the deprecation-drive-5 branch September 11, 2022 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants