chore: WPGraphQLTestcase upgraded to v3 #3116
Conversation
justlevine
changed the title
jasonbahl
left a comment
jasonbahl
left a comment
There was a problem hiding this comment.
Left a couple comments / questions
tests/wpunit/AccessFunctionsTest.php
Outdated
| public function testDeRegisterConnectionWithUnderscores() { | ||
|
|
||
| deregister_graphql_connection( 'test_field_with_underscores' ); | ||
| deregister_graphql_connection( 'TestConnectionFieldWithUnderscores' ); |
There was a problem hiding this comment.
This change defeats the purpose of the test 🤔
There was a problem hiding this comment.
Intent of the test as I understood is that deregister_graphql_connection() also removes the connecting field even if the field is in underscores.deregister_graphql_field() is tested separately for underscores.
There was a problem hiding this comment.
Or maybe it is a type with underscores, and the connectionTypeName needs to be changed to include underscores?
There was a problem hiding this comment.
@justlevine I bet the intent was for the Type Name to have underscores. Because it is valid to have a Type like My_Custom_Type and I believe the intent of the test was to ensure that any normalization on the strings under the hood isn't stripping underscores, etc 🤷🏻♂️
Like I know the registry normalizes types to lowercase strings in some places, so this should be ensuring that a connection registered with underscores is also removed when deregistered
| 'fromFieldName' => 'failingAuthConnection', | ||
| 'resolve' => static function () { | ||
| return [ 'nodes' => [ null, false, 0 ] ]; | ||
| return [ 'nodes' => [ '', false, 0 ] ]; |
There was a problem hiding this comment.
why did this change? Is there a regression somewhere we need to document?
There was a problem hiding this comment.
If you return a null for an object's root you GraphQL does not resolve that types fields. Which was making this test fail.
There was a problem hiding this comment.
Specifically, the Connection Interfaces release made nodes non nullable. The error was just getting suppressed before.
There was a problem hiding this comment.
Verbose pre-change error log in this workflow attempt: https://github.com/wp-graphql/wp-graphql/actions/runs/8918725858/job/24495559399
There was a problem hiding this comment.
Okay, I think I figured out the way the test was suppose to be in the first place and fixed it, and added some inline comments so this never happens again.
There was a problem hiding this comment.
Specifically, the Connection Interfaces release made
nodesnon nullable. The error was just getting suppressed before.
That first node was never supposed to be a falsy value in the first place. I change it to test.
…rCustomConnectionWithAuth
|
Code Climate has analyzed commit 46884ae and detected 0 issues on this pull request. View more on Code Climate. |
4 participants
What does this implement/fix? Explain your changes.
assertIsResponseValidassertion to use the newassertResponseIsValidassertion.Does this close any currently open issues?
Any relevant logs, error output, GraphiQL screenshots, etc?
Any other comments?
…
Where has this been tested?
Operating System: …
WordPress Version: …