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

[Fusion] Add tests for various error cases #7006

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

tobias-tengler
Copy link
Collaborator

@tobias-tengler tobias-tengler commented Mar 21, 2024

This PR adds a number of tests for both the handling of transport and individual subgraph errors. The data portion of the response is as expected in most cases, but errors and especially their paths are incorrect most of the time. This is problematic for error handling clients such as Relay which will expect error paths to the leaf field that has been nulled by the error.
I've also added a new TestSubgraph and TestSubgraphCollection to simplify simulating Fusion composition and execution in the scope of a single test without having to update the Demo Subgraphs used for other integration tests.

This PR doesn't change any functionality, it just adds tests and the expected + current snapshot versions. I plan to use this as a foundation on which to submit subsequent PRs to (hopefully) address all of the issues surfaced by these tests.
I'm sorry for the large diff, but I wanted to have all error cases I could think of covered by tests before modifying the behavior and accidentally changing correct behavior.

3e81e5a: This commit shows what is currently produced, compared to the expected response.

@CLAassistant
Copy link

CLAassistant commented Mar 21, 2024

CLA assistant check
All committers have signed the CLA.

@tobias-tengler tobias-tengler force-pushed the fusion-error-handling branch 3 times, most recently from 69328d3 to af35a84 Compare June 20, 2024 21:13
@ChilliCream ChilliCream deleted a comment from github-actions bot Jun 20, 2024
@tobias-tengler tobias-tengler force-pushed the fusion-error-handling branch 8 times, most recently from 2598fef to 84859e8 Compare June 25, 2024 20:36
@tobias-tengler tobias-tengler changed the title [wip] Add error tests [Fusion] Add tests for various error cases Jun 25, 2024
@tobias-tengler tobias-tengler marked this pull request as ready for review June 25, 2024 20:42
@tobias-tengler tobias-tengler added the 👓 ready-for-review The PR is ready for review. label Jun 25, 2024
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.96%. Comparing base (57637a7) to head (f8359dc).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7006      +/-   ##
==========================================
+ Coverage   75.88%   75.96%   +0.07%     
==========================================
  Files        2376     2679     +303     
  Lines      116553   132758   +16205     
==========================================
+ Hits        88451   100853   +12402     
- Misses      28102    31905    +3803     
Flag Coverage Δ
unittests 75.96% <ø> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@michaelstaib
Copy link
Member

@tobias-tengler have marked the once that are tests that are not ok so we know what to fix?

@tobias-tengler
Copy link
Collaborator Author

It's all but one test that isn't currently failing, mostly because of the errors. I've skipped them all for now though and made the previous .expected file the actual snapshot file to compare against.

@michaelstaib
Copy link
Member

Awesome ... I will have a look at it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants