Skip to content

Conversation

benjaminjkraft
Copy link
Collaborator

@benjaminjkraft benjaminjkraft commented Aug 2, 2025

Two issues here:

I have:

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

Two issues here:
- In the with-config tests, we were generating all the different input
  files into one output. This makes sense in user code, but in our
  tests, we reuse the same types all over the place, so not only is it
  confusing to read all the tests interleaved, it can cause
  nondeterminism anytime our conflict-detection is incomplete (#123).
  As it happens, #308 introduced such a case, so tests started flaking.
  Fixes #281.
- This turned the case introduced by #308 from a flake into a consistent
  failure, which indicates a bug in our option-handling. The bug
  is actually unrelated to the changes there, so for now I just remove
  the test case.
Copy link
Member

@csilvers csilvers left a comment

Choose a reason for hiding this comment

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

I understand how this PR fixes the first issue you mention in the commit message, but I'm not sure I understand the second. I'd like to hold off approving until I do.

I'm guessing that the change to add PokemonInput is what fixes the second issue? How does that fix anything?

@benjaminjkraft
Copy link
Collaborator Author

Yes, sorry, the second fix is to add the PokemonInput type to bindings. That means instead of autogenerating a Go type for that GraphQL type, we use the existing one. Generating the type hit one of the cases we now intentionally validate against (#338 -- where a global omitempty applies to a field that doesn't make sense for); we could fix it in other ways (e.g. an explicitly omitempty: false on that field) but this was more consistent with how the other tests treat this type, and I believe we already have tests for those other cases anyway.

Copy link
Member

@csilvers csilvers left a comment

Choose a reason for hiding this comment

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

ok, sounds good!

@benjaminjkraft benjaminjkraft merged commit 27c858e into main Aug 14, 2025
8 checks passed
@benjaminjkraft benjaminjkraft deleted the benkraft.fix-tests branch August 14, 2025 23:42
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.

Split TestGenerateWithConfig cases
2 participants