-
Notifications
You must be signed in to change notification settings - Fork 114
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
Add integration tests against a gqlgen server #50
Conversation
158b630
to
4ad4590
Compare
// against a real server. | ||
// | ||
// These are especially important for cases where we generate nontrivial logic, | ||
// such as JSON-unmarshaling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a comment pointing about where the test server code is. Otherwise the magic test data appears out of nowhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I appreciate the generation tests, and the test server will most certainly be helpful. I left a comment about perhaps performing some additional validation on the config, but that isn't blocking.
@@ -1,11 +1,11 @@ | |||
# These are the defaults, and are just included to be explicit. | |||
package: example | |||
schema: schema.graphql | |||
queries: | |||
operations: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that generation still works when this key is queries
since genqlient.graphql
is the default value for operations
. What do you think about using yaml.UnmarshalStrict
to detect invalid keys in the configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's probably a good idea. This actually confused me for a while, because I copied this config for the integration tests, but actually wanted to change the value, and of course it didn't work. I'll update it in a separate diff.
We have lots of tests covering codegen, but not a lot that actually run the code. For things where all we do is generate types, that's (mostly) fine (especially now that we actually build the code), but as we generate more nontrivial non-type code we need to actually run it. So I wrote some integration tests that spin up a little gqlgen server, and make calls to it; we can add more over time especially as the JSON marshalling logic gets complex (to support fragments). They're more work to write than the snapshot tests, but of course they can test a lot more. In addition to gqlgen, I pulled in testify assert/require, because I really wanted to be able to use assert.Equal and such for these. I didn't bother converting existing tests, although I assume they will become useful elsewhere in time. Both gqlgen and testify are of course only used in tests. Fixes #21 and #24. Issue: #21 Issue: #24 Test plan: make check
4ad4590
to
e4adf41
Compare
Summary:
We have lots of tests covering codegen, but not a lot that actually run
the code. For things where all we do is generate types, that's (mostly)
fine (especially now that we actually build the code), but as we
generate more nontrivial non-type code we need to actually run it.
So I wrote some integration tests that spin up a little gqlgen
server, and make calls to it; we can add more over time especially as
the JSON marshalling logic gets complex (to support fragments).
They're more work to write than the snapshot tests, but of course they
can test a lot more.
In addition to gqlgen, I pulled in testify assert/require, because I
really wanted to be able to use assert.Equal and such for these. I
didn't bother converting existing tests, although I assume they will
become useful elsewhere in time. Both gqlgen and testify are of course
only used in tests.
Fixes #21 and #24.
Issue: #21
Issue: #24
Test plan:
make check