Skip to content
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

chore(tests): cleaner GraphQL schema snapshots #533

Merged
merged 17 commits into from Oct 15, 2019

Conversation

singingwolfboy
Copy link
Contributor

@singingwolfboy singingwolfboy changed the title Cleaner snapshots for postgraphile-core tests Cleaner snapshots for tests Oct 8, 2019
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Awesome; let's DRY it up and then it's good to merge I think! This makes the test files so much nicer, and now I can literally link people to them as example GraphQL schemas without backslashes everywhere which is just beautiful - thanks again! 🎉

packages/graphile-build/__tests__/enum.test.js Outdated Show resolved Hide resolved
packages/pg-pubsub/__tests__/subs.test.ts Outdated Show resolved Hide resolved
packages/pg-pubsub/__tests__/subs.test.ts Show resolved Hide resolved
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Looking good; just need to undo the schema sorting 👍

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Awesome; this makes the snapshots a hell of a lot more readable 👍

The exclusive input argument for this mutation. An object type, make sure to see documentation for this object’s fields.
"""
input: JsonIdentityMutationInput!
): JsonIdentityMutationPayload
Copy link
Member

Choose a reason for hiding this comment

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

This has been reordered I think?

EDIT: ah, it's because my old schema sorter and GraphQL's differ slightly. This is fine.

@benjie benjie changed the title Cleaner snapshots for tests chore(tests): cleaner GraphQL schema snapshots Oct 15, 2019
@benjie benjie merged commit d407d8e into graphile:master Oct 15, 2019
@ab-pm
Copy link
Contributor

ab-pm commented Oct 18, 2019

Wow, this is awesome! But why don't you want to sort schemas by default?

Btw, in my own project I even omit field descriptions from the schema to get an even cleaner snapshot diff:

import { GraphQLSchema, buildASTSchema, lexicographicSortSchema, parse, printSchema, visit } from 'graphql';
const simplified: GraphQLSchema = lexicographicSortSchema(buildASTSchema(visit(parse(printSchema(schema)), {
	StringValue(node, key) {
		return key == 'description' ? null : node;
	}
})));

Might be interesting to provide as an option.

@singingwolfboy
Copy link
Contributor Author

@ab-pm: quoting @benjie's resolved comment:

Ah; these are now being alphabetically sorted. This can be misleading as order is significant in GraphQL (e.g. it impacts the order that things are show in auto-complete/in the GraphQL docs) and can be used to group related things together (e.g. showing simple fields before showing relations). There are some places where the schema should be compared sorted, and some where order is significant. Typically I want to be aware if the schema order has changed as it will affect people dumping their schema downstream. For now; lets try and match the previous snapshots (but with the unnecessary backslashes removed).

Unfortunately, I don't think that Jest snapshot matchers can expose a configuration interface, so there's no way to provide an option to change these. However, if I'm wrong, then feel free to file an issue on the repository suggesting this change -- or even better, send a pull request! 😄

@ab-pm
Copy link
Contributor

ab-pm commented Oct 21, 2019

Thanks, I had not seen that.

Yeah, it seems Jest snapshot serialisers are not configurable, unless you add them explicitly with expect.addSnapshotSerializer where one could call a function with options that creates the serialiser.

@benjie
Copy link
Member

benjie commented Oct 22, 2019

The field order is officially significant (Lee Byron himself states so); so by default we should honour it. lexicographicSortSchema is always available if you want to compare a sorted schema 👍

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.

None yet

3 participants