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

make output deterministic for graphql interfaces #209

Merged
merged 3 commits into from
Jul 28, 2022

Conversation

vikstrous2
Copy link
Contributor

@vikstrous2 vikstrous2 commented Jul 27, 2022

We noticed that the output from genqlient can be non-deterministic when a graphql query queries a field that's an interface. This PR fixes that by sorting the types as soon as we extract them from the schema.

The snapshots for the tests need to be updated, but I don't know an easy way to do that. Is there a command or a flag that I can just flip somewhere to update them?

I have:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • n/a Included a link to the issue fixed, if applicable
  • n/a Included documentation, for new features
  • Added an entry to the changelog

@StevenACoffman
Copy link
Member

@vikstrous2 Looks like some test failures

@benjaminjkraft
Copy link
Collaborator

Huh, I'm a bit surprised since we've never run into this in the tests (or at Khan as far as I know). I'm guessing the issue is actually if you have multiple schema files and the order they get read in is nondeterministic, or something. But anyway, it makes good sense to make this more stable even if in principle we could solve it elsewhere, so I'm happy to merge once tests are updated.

For the tests, it should just be UPDATE_SNAPSHOTS=1 make check.

Signed-off-by: Steve Coffman <[email protected]>
@StevenACoffman
Copy link
Member

Yeah, I ran UPDATE_SNAPSHOTS=1 make check and then pushed the changed version to this PR to clarify which test failures were not fixed by that.

@benjaminjkraft
Copy link
Collaborator

Ah! It looks like we also just need go generate ./... to update the genqlient-generated files for integration tests.

benjaminjkraft added a commit that referenced this pull request Jul 28, 2022
The generated files for integration tests aren't strictly snapshots and so
`UPDATE_SNAPSHOTS=1` won't work (maybe we should make it work?).
Instead you also need to `go generate ./...`. This came up in #209.
@benjaminjkraft
Copy link
Collaborator

Ok, there we go!

@benjaminjkraft benjaminjkraft merged commit 2ae8ea4 into Khan:main Jul 28, 2022
benjaminjkraft added a commit that referenced this pull request Jul 28, 2022
The generated files for integration tests aren't strictly snapshots and so
`UPDATE_SNAPSHOTS=1` won't work (maybe we should make it work?).
Instead you also need to `go generate ./...`. This came up in #209.
csilvers added a commit that referenced this pull request Feb 26, 2024
In #209, @vikstrous2 added a bugfix to sort the list of interfaces
when generating code, so the generated output was stable.  This worked
great, but there was one more case where we need to do that, involving
interfaces in fragments.
csilvers added a commit that referenced this pull request Feb 26, 2024
…ts. (#323)

In #209, @vikstrous2 added a bugfix to sort the list of interfaces when
generating code, so the generated output was stable. This worked great,
but there was one more case where we need to do that, involving
interfaces in fragments.

I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla)
- [x] Added tests covering my changes, if applicable
- [ ] Included a link to the issue fixed, if applicable
- [ ] Included documentation, for new features
- [x] Added an entry to the changelog
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