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

Expand defer tests #3399

Merged
merged 11 commits into from
Dec 2, 2024
Merged

Conversation

phughes-scwx
Copy link
Contributor

As requested in #3384 (link), this updates the defer tests to cover using the if argument, and the label argument for fragment spreads.

Additionally, this PR:

  • refactors the defer tests into a table test so that we can do more fine-grained, duplicative tests without repeating handler / response parsing boilerplate;
  • updates the defer tests to run every test against both transports that support @defer, not just SSE, to ensure consistency between the implementations;
  • adds a test client helper Client.IncrementalHTTP based on Client.SSE that can be used to test@defer against transport.MultipartMixed.

General notes:

  • I've added a some documentation glosses that may or may not be accurate / useful. Correct me or update as you see fit.
  • I am not totally sure of the multipart/mixed client implementation: did my best on that one. It's in the documentation and TODO notes, but I may have missed the point in some cases.
    • If you want, we can break this part of the PR out and keep the tests focused on SSE for now.
  • In order to reduce duplication for the table tests I made a few choices (like using a generic type and reflect) that can be obscure, and you may not like that and it can be replaced. But my personal preference is to make a clear assertion that is encoded in the table test case struct, and this made it possible with a lot less repetition.

@StevenACoffman @giulio-opal

@phughes-scwx
Copy link
Contributor Author

Not certain why the coverage job is failing: I regenerated the singlefile test server to rename the operations in the test... May be there is some golden test for the server codegen I do not know how to update?

@phughes-scwx
Copy link
Contributor Author

phughes-scwx commented Nov 27, 2024

This Thanksgiving I am grateful for all those who maintain open source projects. 👍

@StevenACoffman
Copy link
Collaborator

Hey, thanks for circling back to this! I'm on my phone, but it looks like something is not quite right with the tests?

@phughes-scwx
Copy link
Contributor Author

Yeah no hurry or anything, but I don't quite know what I'm missing. Getting hundreds of "results did not match" on the schema-generated code for the singlefile test. If I had to guess you have a snapshot/goldenfile test for that code, but I didn't have time to track it down. Will look again today.

@coveralls
Copy link

coveralls commented Dec 2, 2024

Coverage Status

coverage: 59.877% (+0.1%) from 59.732%
when pulling 35b0395 on phughes-scwx:issue-1972-follow-up
into de8ff90 on 99designs:master.

@phughes-scwx
Copy link
Contributor Author

OK, needed to update the followschema codegen test schema to match. Looks good. Will rebase on master but the coverage also looks good.

@StevenACoffman StevenACoffman merged commit 3736848 into 99designs:master Dec 2, 2024
16 of 17 checks passed
@StevenACoffman
Copy link
Collaborator

Thanks! Sorry about the tab character. This is extremely high quality. I really appreciated that you added such nice comments, especially since this is a draft spec, and you provide a nice trail of breadcrumbs for people to update it if the spec changes.

@phughes-scwx phughes-scwx deleted the issue-1972-follow-up branch December 2, 2024 17:50
@StevenACoffman StevenACoffman added the defer-or-stream Related to GraphQL @defer or @stream directives label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defer-or-stream Related to GraphQL @defer or @stream directives
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants