AP-490: Finalize federated service check and add integration tests#1308
Merged
justinanastos merged 14 commits intomasterfrom Jun 13, 2019
Merged
AP-490: Finalize federated service check and add integration tests#1308justinanastos merged 14 commits intomasterfrom
justinanastos merged 14 commits intomasterfrom
Conversation
justinanastos
force-pushed
the
justin/federated-service-check
branch
15 times, most recently
from
7ab8e9f to
95f4245
Compare
justinanastos
changed the title
justinanastos
commented
Jun 6, 2019
justinanastos
force-pushed
the
justin/federated-service-check
branch
from
95f4245 to
a450348
Compare
justinanastos
changed the title
evans
reviewed
Jun 6, 2019
Contributor
evans
left a comment
evans
left a comment
There was a problem hiding this comment.
Some suggestions for to improve the usability and maintainability. Quite possible that some aren't possible in the existing world.
| throw new Error("No SDL found for federated service"); | ||
| } | ||
|
|
||
| task.output = `Attempting to compose graph with ${chalk.blue( |
Contributor
There was a problem hiding this comment.
It would be great to have some diagnostic information about how many other services are included in the composition
Contributor
Author
There was a problem hiding this comment.
Unloaded question: why do you think this is important @evans ?
Contributor
There was a problem hiding this comment.
Good question! I think it's a nice to have, which would indicate how much work apollo is doing and a nice way for users to understand that the command is working as intended
justinanastos
force-pushed
the
justin/federated-service-check
branch
from
a450348 to
ef31e2a
Compare
justinanastos
requested review from
evans and
trevor-scheer
and removed request for
evans
trevor-scheer
approved these changes
Jun 12, 2019
I left a few small ones that won't be changing. There's a bug right now causing inline snapshots to not be mutable. Let's stick with regular snapshots until we resolve that.
This only appears in tests when we capture stdout. We don't want the times in our tests!
We shouldn't be forcing a type cast; we should instead check and guarantee the type.
`configName` is ambiguous in the world where we're now considering the result of composition a "graph" and the partial schemas that comprise a federated graph "service."
We'll use this to capture stdout in our integration tests. This library was chosen only because `@oclif/test` is already using it.
Per Matt's instructions; this will also remove the `--federated` flag in place of `--serviceName` implying federation.
Allowing Listr to chose it's own renderer can cause insidious bugs. For example, bash and zsh will choose different renderers for tests, causing unpredictable outputs. 1. We don't want to show a spinner in tests 2. We want to see individual changes to titles and output lines; this is accomplished with the verbose renderer. Note that this _must_ be override-able because some functions require the `silent` renderer. See https://github.com/SamVerschueren/listr#renderer
justinanastos
force-pushed
the
justin/federated-service-check
branch
from
ef31e2a to
a12eab0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review this PR commit-by-commit
This PR:
service:checknock--federatedflag in place of implying federated behavior with--serviceName--markdownand--jsonare not supported; but that's coming soon.When creating new responses, use nock's recorder functionality (https://github.com/nock/nock#recording). We made the request matching only check the query name so the code would be less brittle.
TODO:
nockrecorder functionality :)