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

test(graphql): add graphql unit tests #1060

Merged
merged 7 commits into from
Sep 6, 2022

Conversation

maxcao13
Copy link
Member

Fixes #947

@maxcao13 maxcao13 added the test label Aug 30, 2022
@maxcao13
Copy link
Member Author

maxcao13 commented Sep 2, 2022

I have written tests for the fetcher and mutator implementations and surface level tests for the handlers. I have a few questions overall.

  1. Why doesn't the AllArchivedRecordingFetcherimplement AbstractPermissionedDataFetcher? I'm assuming we need permissions to view targets and recordings from the AllArchives view just like the other fetchers.
  2. Is the GraphiQL handler for devs to be able to graphically see our GraphQL queries and responses? If so, does it still make sense to test it?
  3. Speaking of the handlers, I'm not sure how to test the GraphQLGetHandler and Post handlers 'correctly' since we don't override the handle method of the vert.x RequestHandlers and most of the graphql implementation is very abstracted and internal to vertx and graphql itself. Does it still make sense to try and test the handler endpoints as unit tests and get actual responses, feasibly? I was thinking the integration tests also does this anyways.

@maxcao13 maxcao13 marked this pull request as ready for review September 2, 2022 23:03
@andrewazores
Copy link
Member

andrewazores commented Sep 6, 2022

  1. Good question. It should. Hareet wrote that fetcher around the same time that I wrote the abstract permissioned base class, so I think I just missed including this class when I rebased my work on top.
  2. Correct, it gives a REPL-like page where you can tinker with GraphQL queries. There's no need to test it.
  3. These can have very basic unit tests just to ensure that they have the correct HTTP verb, correct path, and require authorization for example. There is no need to try to test them any deeper than that since it just ends up delegating to a Vert.x implementation and as you say, we exercise that already with the integration tests too.

@maxcao13
Copy link
Member Author

maxcao13 commented Sep 6, 2022

I've fixed everything that I can see and most of the GraphQL stuff should be covered.

Thoughts or anything I've missed?

@andrewazores
Copy link
Member

Awesome, thanks for doing all of this.

@andrewazores andrewazores merged commit b16853b into cryostatio:main Sep 6, 2022
@maxcao13 maxcao13 deleted the graph-ql-tests branch September 7, 2022 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Unit tests for GraphQL
2 participants