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

Values of correct type when validating SDL. Fixes #965 #966

Merged
merged 2 commits into from
Feb 24, 2023

Conversation

maloku
Copy link
Contributor

@maloku maloku commented Feb 22, 2023

No description provided.

@maloku
Copy link
Contributor Author

maloku commented Feb 23, 2023

@yanns do you think these tests are because of the changes I made? If I revert my changes, some tests do fail. Am I missing something?

@yanns
Copy link
Contributor

yanns commented Feb 23, 2023

For me, all tests are passing on main. It's also a prerequisite for any PR to be merged.
When I checkout your PR, sangria.schema.AstSchemaMaterializerSpec is failing. So it should be related to your changes.
But don't panic: check how the tests are failing. Maybe we should update the tests if they show an issue.

…StringValue) to be checked with invalid types.

Signed-off-by: Kadri Nebiu <[email protected]>
@maloku
Copy link
Contributor Author

maloku commented Feb 24, 2023

Oh - that was a good catch. My change ended up validating comments which are ast.StringValue with the type of argument which contains the default value. I adopted it now, and the failing test AstSchemaMaterializerSpec is passing.

When I run sbt test I get 4 test failed on main - this is the reason I asked because I expected some test to fail:

[info] Tests: succeeded 1246, failed 4, canceled 0, ignored 1, pending 0
[info] *** 4 TESTS FAILED ***
[error] Failed tests:
[error]         sangria.marshalling.QueryAstMarshallingSupportSpec
[error]         sangria.macros.LiteralMacroSpec
[error]         sangria.renderer.SchemaRenderSpec

Let's see now

@yanns
Copy link
Contributor

yanns commented Feb 24, 2023

When I run sbt test I get 4 test failed on main - this is the reason I asked because I expected some test to fail:

[info] Tests: succeeded 1246, failed 4, canceled 0, ignored 1, pending 0
[info] *** 4 TESTS FAILED ***
[error] Failed tests:
[error]         sangria.marshalling.QueryAstMarshallingSupportSpec
[error]         sangria.macros.LiteralMacroSpec
[error]         sangria.renderer.SchemaRenderSpec

Can you check that further? It's not expected.

Copy link
Contributor

@yanns yanns left a comment

Choose a reason for hiding this comment

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

I really appreciate the attention you gave to the tests! 💯
As I'm not completely sure about the impact of this change, I'll do the following:

  • merge this PR
  • cut a RC release
  • try this RC release (you can also try the RC release if you want)
    If everything is good, then we'll go with a stable release.
    If we notice an issue, we can check it.

@yanns yanns merged commit c558fbc into sangria-graphql:main Feb 24, 2023
@yanns
Copy link
Contributor

yanns commented Feb 24, 2023

I've tried https://github.com/sangria-graphql/sangria/releases/tag/v3.5.3-RC1 on internal applications, and it does not break anything ;)

@maloku
Copy link
Contributor Author

maloku commented Feb 24, 2023

Great! I do not have a production application as I am only learning (in detail) GraphQL and just happened to pick Scala and Sangria for this journey. But I am still at the beginning, so I may come back with additional stuff, who knows? Thank you so much for supporting me.

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.

2 participants