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

Consider removing apollo-server-testing #4952

Closed
glasser opened this issue Feb 23, 2021 · 5 comments · Fixed by #5238
Closed

Consider removing apollo-server-testing #4952

glasser opened this issue Feb 23, 2021 · 5 comments · Fixed by #5238

Comments

@glasser
Copy link
Member

glasser commented Feb 23, 2021

apollo-server-testing provides a very thin wrapper around ApolloServer.executeOperation (which was introduced specifically to be used by apollo-server-testing. It's not clear to me that it's independently valuable, vs just documenting executeOperation.

@glasser glasser added this to the Release 3.x milestone Feb 23, 2021
@glasser
Copy link
Member Author

glasser commented Feb 24, 2021

Additionally, many users want to test their actual HTTP pipeline rather than direct execution. There's a separate library https://github.com/zapier/apollo-server-integration-testing that (for Express users at least) supports this. It may be the right move to endorse this package as a replacement.

@glasser
Copy link
Member Author

glasser commented May 2, 2021

This card is largely just getting rid of a-s-t in the docs, which we could quickly do in AS2 too.

@moberegger
Copy link

moberegger commented May 4, 2021

Hey @glasser. I have been spending a lot of time recently working on integration tests, so I have some thoughts on this if you don't mind me sharing.

I have read through the issue that motivated this ticket, so am aware of the problem. We were using apollo-server-testing to do "integration" tests, and encountered the same problems where the tests wouldn't go through any of our middleware, so things like validating access tokens and initialize the context weren't being done out of the box for us. We came up with a simple solution that would allow tests to spin up their own ApolloServer and inject stuff directly into the context. Our solution wasn't too dissimilar from some of the suggestions in #2277.

We wanted to improve our integration tests to be representative of production behaviour, and based on this ticket and the issue earlier, I tried using https://github.com/zapier/apollo-server-integration-testing as suggested. What I learned is that that library isn't doing "real" integration tests either; it is simply providing an API to more conveniently provide the req to the context (which are also mocks). It's actually quite similar to what we were already doing. Similar to apollo-server-testing, it runs the queries directly against the ApolloServer execution engine, and does not run through any middleware or http layer. This isn't bad, but it is also not a "real" integration test either.

What I ended up doing was building a very thin wrapper around supertest that can accepts an express app and runs a GraphQL query through the whole thing. I made that API very similar to that provided by apollo-server-testing so the refactor steps were pretty simple. By doing it this way, you can run your integration tests through the entire stack, with the requests and context being handled for you as they would in production. It was also faster because it is quite expensive to spin up to spin up a new ApolloServer instance for each test; using this wrapper, we could use the same app across all of our tests, because the context runs just like it does in production.

If you like, I can try to "isolate" this utility, and maybe this can become a core maintained @apollo/server-integration-testing package or something. While I could provide this as a community package, I personally see a lot of value in having something like this in core. It makes it more discoverable and gives developers more confidence. In my experienced, not having core solutions for "solved problems" like that can lead to friction and FUD. For example, Meteor (yes, I have been following your work for that long... big fan, by the way!) has gone through this pain, and it is clear to me that getting a stamp of approval on something really mitigates developers' fears. Just my opinion, of course 😄 .

I can submit a (perhaps rough) PR for this, if this is something you would be interested in. I don't want to tell you what to do or how to maintain your project, but maybe I can try to change your mind 😛 . Let me know if that interests you at all. If it does... please bear with me, as I am a bit of an OS newbie, so this type of thing is a bit daunting for me... but I'm sure I can get over it.

I would also ask that you reconsider deprecating apollo-server-testing. I still think it is useful for testing things like custom scalars and such; things that don't need to run against your app/schema. If you are confident that documenting ApolloServer.executeOperation would yield the same benefits, I don't want to discourage you; I suppose my main concern is suddenly losing that nicety of being able to intuitively test small pieces like that.

@glasser
Copy link
Member Author

glasser commented May 4, 2021

@moberegger Thanks for the feedabck!

The package you mentions sounds pretty useful. I think a good pattern here might be for you to publish it under your own namespace (or top level) and we can recommend it in docs; if it becomes quite popular then we could do a "friendly takeover".

I don't follow your concern about custom scalars — how does the separate package apollo-server-testing help with those in a way that executeOperation doesn't?

@moberegger
Copy link

moberegger commented May 5, 2021

I don't follow your concern about custom scalars — how does the separate package apollo-server-testing help with those in a way that executeOperation doesn't?

I actually don't know, I just know that it currently works well for things like that. I was more so addressing "It's not clear to me that it's independently valuable" and I was trying to give an example of a situation where it is valuable.

I am very unfamiliar with the inner workings of Apollo Server, so I don't understand what "documenting executeOperation" ultimately means, or if it means that the same things won't possible; my initial thought was that I would have to understand core implementation details to do testing, which felt a bit daunting. If that's not the case and it does the same thing with a superficial API change, then my worries are unfounded and it's just uncertainty on my part. If that's the case, please ignore my ramblings!

glasser added a commit that referenced this issue May 25, 2021
The `apollo-server-testing` package exports one small function which is
just a tiny wrapper around `server.executeOperation`. The one main
advantage it provides is that you can pass in operations as ASTs rather
than only as strings.

This extra layer doesn't add much value but does require us to update
things in two places (which cross a package barrier and thus can be
installed at skewed versions). So for example when adding the second
argument to `executeOperation` in #4166 I did not bother to add it to
`apollo-server-testing` too. We've also found that users have been
confused by the `createTestClient` API (eg #5111) and that some linters
get confused by the unbound methods it returns (#4724).

So the simplest thing is to just teach people how to use the real
`ApolloServer` method instead of an unrelated API.

This PR allows you to pass an AST to `server.executeOperation` (just
like with the `apollo-server-testing` API), and changes the docs to
recommend `executeOperation` instead of `apollo-server-testing`. It also
makes some other suggestions about how to test Apollo Server code in a
more end-to-end fashion, and adds some basic tests for
`executeOperation`.

Fixes #4952.
Apollo Server 3 automation moved this from Straightforward breaking changes to Done May 25, 2021
glasser added a commit that referenced this issue May 25, 2021
The `apollo-server-testing` package exports one small function which is
just a tiny wrapper around `server.executeOperation`. The one main
advantage it provides is that you can pass in operations as ASTs rather
than only as strings.

This extra layer doesn't add much value but does require us to update
things in two places (which cross a package barrier and thus can be
installed at skewed versions). So for example when adding the second
argument to `executeOperation` in #4166 I did not bother to add it to
`apollo-server-testing` too. We've also found that users have been
confused by the `createTestClient` API (eg #5111) and that some linters
get confused by the unbound methods it returns (#4724).

So the simplest thing is to just teach people how to use the real
`ApolloServer` method instead of an unrelated API.

This PR allows you to pass an AST to `server.executeOperation` (just
like with the `apollo-server-testing` API), and changes the docs to
recommend `executeOperation` instead of `apollo-server-testing`. It also
makes some other suggestions about how to test Apollo Server code in a
more end-to-end fashion, and adds some basic tests for
`executeOperation`.

Fixes #4952.
@glasser glasser removed this from the Release 3.x milestone Jun 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants