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: introduce tests for @requires #704

Closed

Conversation

clayne11
Copy link
Contributor

@clayne11 clayne11 commented Apr 7, 2024

Motivation and Context

The Cosmo router ostensibly supports the @requires directive, however it doesn't seem to work properly. Added tests to router-tests to show the expected behavior.

These tests currently fail since the router logic seems to be incorrect. This is both true for @requires directives with flat fields as well as ones with nested selection sets, although each of these lead to different error patterns.

@Aenimus
Copy link
Member

Aenimus commented Apr 11, 2024

Hi @clayne11

Would you mind rebasing? If things are still failing, we'll investigate.

Thank you for the contribution!

@clayne11 clayne11 force-pushed the curtis.layne/update-gqlgen-2 branch from 017a5c3 to fafc4b9 Compare April 12, 2024 18:23
Currently it does not work as expected.
@clayne11 clayne11 force-pushed the curtis.layne/update-gqlgen-2 branch from fafc4b9 to c78416c Compare April 12, 2024 18:25
@clayne11
Copy link
Contributor Author

clayne11 commented Apr 12, 2024

Rebased — tests are still failing.

--- FAIL: TestTestdataQueries (5.67s)
    --- FAIL: TestTestdataQueries/requires (0.31s)
        integration_test.go:469: Result did not match the golden fixture. Diff is below:

            --- Expected
            +++ Actual
            @@ -1,16 +1,11 @@
             {
            -  "data": {
            -    "products": [
            -      {
            -        "lead": {
            -          "id": 1,
            -          "derivedID": 1
            -        },
            -        "isLeadAvailable": false
            -      },
            -      {},
            -      {}
            -    ]
            -  }
            +  "errors": [
            +    {
            +      "message": "Cannot return null for non-nullable field 'Query.products'.",
            +      "path": [
            +        "products"
            +      ]
            +    }
            +  ],
            +  "data": null
             }
            -

Copy link

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 27, 2024
@clayne11
Copy link
Contributor Author

This is not stale, please don't close it.

@Aenimus
Copy link
Member

Aenimus commented Apr 30, 2024

@clayne11 Hi Curtis,

Apologies for my lack of updates. We know what the issue is, and we're working on some fixes.

We're still interested in the PR and don't plan to close this.

Thanks for your patience!

@github-actions github-actions bot removed the Stale label May 1, 2024
@github-actions github-actions bot added the Stale label May 15, 2024
@devsergiy devsergiy removed the Stale label May 28, 2024
@wundergraph wundergraph deleted a comment from github-actions bot May 28, 2024
@devsergiy
Copy link
Member

Hi @clayne11 we have merged a fix for an issue preventing from running tests

I split requires tests into 2 separate tests and fixed requires resolvers
One of the tests is still not working, we could plan it, but at the moment the resolver is missing information about how to execute fetches in the proper order

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.

None yet

3 participants