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.

Sign up for GitHub

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

Jump to bottom

test: Provide tests for client introspection query #1492

Merged
merged 6 commits into from
May 12, 2023
Merged

test: Provide tests for client introspection query #1492

merged 6 commits into from
May 12, 2023

Conversation

orpheuslummis
Copy link
Contributor

Relevant issue(s)

Resolves #1099

Description

Provide test that is somewhat isomorphic to the way that clients validate the resulting introspection schema.

I've found that the client introspection query is the same accross Altair, GraphiQL, and Postman clients.

Note that the test TestClientIntrospectionWithOneToManySchema fails because of issue #1463.
Question: should it be fixed before this PR or should the test be disabled?

Feedback welcome!

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Linux & CI

@orpheuslummis orpheuslummis added area/query Related to the query component area/testing Related to any test or testing suite action/no-benchmark Skips the action that runs the benchmark. labels May 11, 2023
@orpheuslummis orpheuslummis added this to the DefraDB v0.5.1 milestone May 11, 2023
@orpheuslummis orpheuslummis self-assigned this May 11, 2023
@orpheuslummis orpheuslummis requested a review from a team May 11, 2023 01:26
@fredcarle
Copy link
Collaborator

Question: should it be fixed before this PR or should the test be disabled?

I think we can merge it if you change the expected behaviour to returning an error and add a comment saying that this should pass.

@orpheuslummis orpheuslummis force-pushed the orpheus/test/client-introspection-test branch from 853bb0e to 143a4cf Compare May 11, 2023 19:09
@codecov
Copy link

codecov bot commented May 11, 2023 •
edited
Loading

Codecov Report

Merging #1492 (ea9f626) into develop (a49dbed) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1492      +/-   ##
===========================================
- Coverage    72.21%   72.18%   -0.04%     
===========================================
  Files          185      185              
  Lines        18239    18239              
===========================================
- Hits         13171    13165       -6     
- Misses        4029     4033       +4     
- Partials      1039     1041       +2

see 4 files with indirect coverage changes

@orpheuslummis orpheuslummis force-pushed the orpheus/test/client-introspection-test branch from 267bf6c to a2b841e Compare May 12, 2023 03:07
AndrewSisley
AndrewSisley requested changes May 12, 2023
View reviewed changes
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

Looks really good Orpheus, and should help encourage us to expand the schema/introspection tests. Added a few todos, but they are all very localized/small.

tests/integration/client_introspection/client_introspection_test.go Outdated Show resolved Hide resolved
tests/integration/client_introspection/client_introspection_test.go Outdated
testUtils.ClientIntrospectionRequest{
Request: clientIntrospectionQuery,
ExpectedError: "Unknown kind of type: ",
// ExpectedError: "InputFields are missing",
Copy link
Contributor

Choose a reason for hiding this comment

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

todo: Please remove the commented out code (or document if it is there for a reason)

Copy link
Contributor

Choose a reason for hiding this comment

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

please dont forget about this one :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea to have all the TODOs linked to an issue. IMO it should apply retroactively as well.

tests/integration/test_case.go Show resolved Hide resolved
tests/integration/utils2.go Show resolved Hide resolved
tests/integration/client_introspection/client_introspection_test.go Outdated Show resolved Hide resolved
tests/integration/client_introspection/client_introspection_test.go Outdated
},
testUtils.ClientIntrospectionRequest{
Request: clientIntrospectionQuery,
ExpectedError: "Unknown kind of type: ",
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Do you know where/why/how this is erroring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, I'm confused as to which part of the schema + introspection query this is coming from !

Copy link
Contributor

Choose a reason for hiding this comment

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

okay no worries :)

AndrewSisley
AndrewSisley approved these changes May 12, 2023
View reviewed changes
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

LGTM, please dont forget the last todo though before merge

@orpheuslummis orpheuslummis merged commit 8f72069 into sourcenetwork:develop May 12, 2023
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@orpheuslummis
test: Provide tests for client introspection query (sourcenetwork#1492)
a211bee 
## Relevant issue(s)

Resolves sourcenetwork#1099

## Description

Provide test that is somewhat isomorphic to the way that clients
validate the resulting introspection schema.

I've found that the client introspection query is the same accross
Altair, GraphiQL, and Postman clients.

## How has this been tested?

Linux & CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndrewSisley AndrewSisley AndrewSisley approved these changes

Assignees

@orpheuslummis orpheuslummis

Labels
action/no-benchmark Skips the action that runs the benchmark. area/query Related to the query component area/testing Related to any test or testing suite
Projects
None yet
Milestone
DefraDB v0.5.1
Development

Successfully merging this pull request may close these issues.

Compatibility with GraphQL clients introspection queries in our integration tests
3 participants
@orpheuslummis @fredcarle @AndrewSisley