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

Example for doing a full schema unit test #1154

Open
wants to merge 7 commits into
base: 8.x-4.x
Choose a base branch
from

Conversation

pmelab
Copy link
Contributor

@pmelab pmelab commented Feb 1, 2021

Since I was asked how to do it and realised we have no canonical example or documentation.

@pmelab pmelab requested a review from klausi February 1, 2021 07:28
@pmelab pmelab temporarily deployed to mirror February 1, 2021 07:53 Inactive
@klausi klausi added the 4.x label Feb 1, 2021
Copy link
Contributor

@klausi klausi left a comment

Choose a reason for hiding this comment

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

Looks good to me! PHPStan is failing because of #1157 , fixing that there.

use Drupal\Tests\graphql\Kernel\GraphQLTestBase;
use Drupal\user\Entity\User;

class ExampleSchemaTest extends GraphQLTestBase {

This comment was marked as resolved.

@klausi klausi temporarily deployed to mirror February 1, 2021 17:23 Inactive
@klausi
Copy link
Contributor

klausi commented Feb 1, 2021

We have a couple of exceptions in phpcs.xml.dist for the example module, so you also need to fix the paths there.

The other coding standard fixes should be straightforward.

@pmelab pmelab temporarily deployed to mirror February 2, 2021 10:02 Inactive
@pmelab pmelab temporarily deployed to mirror February 2, 2021 10:04 Inactive
Comment on lines 72 to 85
$response = $this->query('{ articles { total, items { title, author } } }');
$content = json_decode($response->getContent(), TRUE);
$this->assertEquals([
'data' => [
'articles' => [
'total' => 3,
'items' => [
['title' => 'ONE', 'author' => 'A'],
['title' => 'TWO', 'author' => 'B'],
['title' => 'THREE', 'author' => 'A'],
],
],
],
], $content);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of $this->query I think we should use $this->assertResults.

The benefits are that it bypasses the HTTP stack which is slightly faster. It also handles the response decoding for you and allows you to test for caching metadata.

Suggested change
$response = $this->query('{ articles { total, items { title, author } } }');
$content = json_decode($response->getContent(), TRUE);
$this->assertEquals([
'data' => [
'articles' => [
'total' => 3,
'items' => [
['title' => 'ONE', 'author' => 'A'],
['title' => 'TWO', 'author' => 'B'],
['title' => 'THREE', 'author' => 'A'],
],
],
],
], $content);
$this->assertResults(
'{ articles { total, items { title, author } } }',
[],
[
'data' => [
'articles' => [
'total' => 3,
'items' => [
['title' => 'ONE', 'author' => 'A'],
['title' => 'TWO', 'author' => 'B'],
['title' => 'THREE', 'author' => 'A'],
],
],
],
]
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right @Kingdutch ! 🤦 Thanks for the input.

@pmelab pmelab temporarily deployed to mirror February 3, 2021 12:26 Inactive
@Kingdutch
Copy link
Contributor

Another thought: should we be excluding this test from the test runs? If we're going to provide an example, it'd be good to just run the test, this way we also know when the documentation/example breaks :D Otherwise we may end up with someone copying a test that doesn't work in the first place.

@klausi
Copy link
Contributor

klausi commented Feb 3, 2021

Yep, we absolutely should execute this test! I think that is done with how we run phpunit on github actions (by just pointing at the graphql module), but I did not double-check.

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