Skip to content

Conversation

@bdunogier
Copy link
Contributor

@bdunogier bdunogier commented Jul 1, 2019

Features

Feature: Schema generation
In order to use GraphQL
As an application maintainer
I need to generate the schema

TODO

Copy link
Member

@mnocon mnocon 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! Couple of comments, mostly about constistency with other Behat tests.

Scenario: An application maintainer generates the schema
Given the schema has not been generated
When I run the command "ezplatform:graphql:generate-schema"
When I clear the cache
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: usually we replace the second consecutive usage with And (And I clear the cache)

paths:
- '%paths.base%/vendor/ezsystems/ezplatform-graphql/features/Generator.feature'
contexts:
- GeneratorContext
Copy link
Member

Choose a reason for hiding this comment

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

Usually we keep the Context classes in Context directories, also define namespaces for them - I think it would be useful to do that here as well, to keep it consistent.

* @license For full copyright and license information view LICENSE file distributed with this source code.
*/

class CacheContext implements \Behat\Symfony2Extension\Context\KernelAwareContext
Copy link
Member

Choose a reason for hiding this comment

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

Why no import? 😄

/**
* @var \Symfony\Component\HttpKernel\KernelInterface
*/
private $kernel;
Copy link
Member

Choose a reason for hiding this comment

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

This is a matter of taste, but I prefer to use KernelDictionary (https://github.com/Behat/Symfony2Extension/blob/master/src/Behat/Symfony2Extension/Context/KernelDictionary.php), as it results in less boilerplate (no need to add a setter)

}

/**
* @Given /^the schema has not been generated$/
Copy link
Member

Choose a reason for hiding this comment

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

Usually I struggle with steps like this one, because I understand them differently when they're used as Given or Then (and then I have to look at the code what they do).

The way I see it:
Given the schema has not been generated - check that the directory is empty, remove files if they exist
Then the schema has not been generated - assert that the directory is empty, fail the step if the files are there

Of course this is a minor thing, because I don't imagine people using Then the schema has not been generated, but I'm curious what's your take on this 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants