Skip to content

Conversation

@shalarewicz
Copy link
Collaborator

@shalarewicz shalarewicz commented Jul 9, 2022

Summary

This updates the test suite for queries using fragments. These tests test for the following:

  • Cases where the fragment has a complexity of 0
  • Cases where the fragment contains a nested object and has non-zero complexity
  • Cases where variables are used on fragment queries
  • Fragment complexity is recomputed for each query
  • Reassess tests for inline fragments.
  • Adds tests for inline fragments on Union and Interface types.

The test also cleans up function mocks in the complexity suite to avoid mocking incorrect return values when tests are run in parallel.

Type of Change

  • Updated test cases

Issues

Evidence

  • Tests for fragments have reasonable failures with the exception of the 'that have a complexity of zero' tests which passes as no fragment complexity is computed and has the same effect as a complexity of 0.

@shalarewicz shalarewicz requested a review from evanmcneely July 9, 2022 21:06
@shalarewicz
Copy link
Collaborator Author

@evanmcneely Do you think the complexity analysis algo should throw an error for unhandled node cases during development? This could help us catch cases we haven't considered. I'm thinking of the case where the test 'with fragments...that have a complexity of zero' is passing before fragments have been implemented?

@evanmcneely
Copy link
Contributor

@evanmcneely Do you think the complexity analysis algo should throw an error for unhandled node cases during development? This could help us catch cases we haven't considered. I'm thinking of the case where the test 'with fragments...that have a complexity of zero' is passing before fragments have been implemented?

haha, while in development it's probably not a big deal but if there's anything we don't get done before "launch" then ya we probably should. It wouldn't be hard because you could throw on error in the else statement wherever there are cases we aren't handling. If something happens we don't expect throw an error.

This might also be a good way to lay out the rest of the node functions and flow of the calls before implementing the logic.

Copy link
Contributor

@evanmcneely evanmcneely left a comment

Choose a reason for hiding this comment

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

This is good. I reviewed the non-null tests in the other PR #64

@shalarewicz
Copy link
Collaborator Author

@evanmcneely Do you think the complexity analysis algo should throw an error for unhandled node cases during development? This could help us catch cases we haven't considered. I'm thinking of the case where the test 'with fragments...that have a complexity of zero' is passing before fragments have been implemented?

haha, while in development it's probably not a big deal but if there's anything we don't get done before "launch" then ya we probably should. It wouldn't be hard because you could throw on error in the else statement wherever there are cases we aren't handling. If something happens we don't expect throw an error.

This might also be a good way to lay out the rest of the node functions and flow of the calls before implementing the logic.

Will add these errors along with the implementation of #44

@shalarewicz shalarewicz marked this pull request as ready for review July 10, 2022 03:02
}
}`;
// Query 1 + 1 hero + max(Droid 0, Human 3) = 5
mockHumanFriendsFunction.mockReturnValueOnce(3);
Copy link
Contributor

Choose a reason for hiding this comment

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

The friends field on human invokes the mockChracterFriendsFunction not the mockHumanFriendFunction

hero(episode: EMPIRE) {
... {
name
scalarList(first: 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is scalar list coming from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The tests for inline fragments on interfaces use the type weight object defined at the top of the file which contains a scalarList field on the Character interface. Let me know if you think it's more clear to create a dedicated type weight object for these tests similar to the tests for inline fragments on unions.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I see, I don't think you need to create a new type weight object, that's just more work to maintain. It's just hard to follow the nested describes in a file this big using githubs GUI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Amen, It could probably benefit from modularity and splitting into multiple files with a shared initialize() method that builds the typeWeight object

Copy link
Contributor

@evanmcneely evanmcneely left a comment

Choose a reason for hiding this comment

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

Good work. Lot's of edge cases I didn't know about. Approve with some things to check.

In implementation, I think you'll run into some problems when looking in the typeWeightsObject for a union and not seeing what it is a union of in order to find the fields you need to get weights of. I wouldn't be surprised if you have to refactor the typeWeightsObject to have that information. We'll only find out when actually doing the work.

@shalarewicz
Copy link
Collaborator Author

Good work. Lot's of edge cases I didn't know about. Approve with some things to check.

In implementation, I think you'll run into some problems when looking in the typeWeightsObject for a union and not seeing what it is a union of in order to find the fields you need to get weights of. I wouldn't be surprised if you have to refactor the typeWeightsObject to have that information. We'll only find out when actually doing the work.

Agreed, see my comments on #44. Current idea is just to look for common fields when we parse the schema and add these to both the Union type SearchResult and the individual types Droid and Human.

@shalarewicz shalarewicz merged commit 39dc915 into dev Jul 15, 2022
@shalarewicz shalarewicz deleted the sh/fragment-tests branch July 15, 2022 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include Fragments in query complexity tests

4 participants