Skip to content

Conversation

JCMais
Copy link
Contributor

@JCMais JCMais commented Jun 26, 2018

Instead of generating the schema by using the type constructors, this changes the tests to use buildSchema with the correct SDL for each test.

Related to #1341

SDL was generated using the following code:

import { printSchema } from './index';

console.log(
  '    const oldSchema = buildSchema(`\n' +
  printSchema(oldSchema).split('\n').map(s => s.padStart(s.length + 6)).slice(0, -1).join('\n') +
  '\n    `);',
);
console.log('---');
console.log(
  '    const newSchema = buildSchema(`\n' +
  printSchema(newSchema).split('\n').map(s => s.padStart(s.length + 6)).slice(0, -1).join('\n') +
  '\n    `);',
);

So it's based solely on how the previous oldSchema and newSchema of each test case were printed by printSchema.

Instead of generating the schema by using the Type constructors,
 use buildSchema with the correct SDL for each test.
},
});
const directiveRemovedLocationOld = new GraphQLDirective({
name: 'Directive Name',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This directive had incorrect name, according to the spec, spaces should not be allowed on directive name:
https://facebook.github.io/graphql/draft/#DirectiveDefinition
https://facebook.github.io/graphql/draft/#Name

Copy link
Member

Choose a reason for hiding this comment

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

👍

{
type: BreakingChangeType.DIRECTIVE_REMOVED,
description: 'skip was removed',
description: 'DirectiveThatIsRemoved was removed',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was necessary since printSchema does not print GraphQL default directives, so I've added a directive to test the same scenario.

Copy link
Member

@IvanGoncharov IvanGoncharov Jun 28, 2018

Choose a reason for hiding this comment

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

Makes sense 👍

{
type: BreakingChangeType.DIRECTIVE_REMOVED,
description: `${GraphQLIncludeDirective.name} was removed`,
description: `DirectiveThatIsRemoved was removed`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change made for the same motive than above, printSchema does not print GraphQL default directives

Copy link
Member

Choose a reason for hiding this comment

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

👍

locations: [DirectiveLocation.FIELD_DEFINITION],
args: {
arg1: {
name: 'arg1',
Copy link
Contributor Author

@JCMais JCMais Jun 26, 2018

Choose a reason for hiding this comment

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

FYI this was giving undefined type to arg1, so I've added something as type for that arg on the SDL below.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@JCMais
Copy link
Contributor Author

JCMais commented Jun 26, 2018

I have not changed the test 'should detect if a directive was implicitly removed' to use buildSchema since it kinda of is testing internal GraphQLSchema behaviour. So I've kept it the way it is.

Another point is that the test should detect locations removed directives within a schema has the test coverage for should detect locations removed from a directive, so IDK if it's necessary to have both.

type Query {
field1: String
}
Copy link
Member

Choose a reason for hiding this comment

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

It's very hard to read when Query is in the middle of SDL.
Looking at other tests it looks like the current convention is too put Query as the last type. Can you please do the same in rest of tests?

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jun 28, 2018

@JCMais Thanks for PR 👍
Everything looks good.
There are some things that could be improved but they are unrelated to SDL conversion.
However, there is one important problem that printSchema output types sorted by names and that makes the test harder to read.
Types could be rearranged in separate PR but it will complicate commit history.
So it would be great if you rearrange types in order that makes sense when you read SDL top to bottom.

@JCMais
Copy link
Contributor Author

JCMais commented Jul 2, 2018

@IvanGoncharov I've changed the ordering of the types, can you check again if it's more readable?

const oldSchema = buildSchema(`
type Type1 {
field1(
arg1: String, arg2: String, arg3: [String], arg4: String, arg5: String!, arg6: String!, arg7: [Int]!,
Copy link
Member

Choose a reason for hiding this comment

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

nit: one argument per line without commas would be consistent with prettier:
https://github.com/prettier/prettier/blob/master/tests/graphql_arguments/__snapshots__/jsfmt.spec.js.snap#L12-L16

Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

Looks great!
@leebyron @mjmahone Would be great if you review this PR so it doesn't block #1341 ?

@mjmahone
Copy link
Contributor

Thanks for making these tests more readable! I had been doing this with my own tests for awhile, so it's great to see this clear improvement in the main repo.

@mjmahone mjmahone merged commit 00e40d1 into graphql:master Jul 16, 2018
@JCMais JCMais deleted the feat/improve-findBreakingChange-tests branch July 16, 2018 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants