Skip to content

Conversation

pmelab
Copy link
Contributor

@pmelab pmelab commented Jan 29, 2018

No description provided.

@codecov
Copy link

codecov bot commented Jan 30, 2018

Codecov Report

Merging #489 into 8.x-3.x will decrease coverage by 0.72%.
The diff coverage is 99.27%.

Impacted file tree graph

@@             Coverage Diff             @@
##           8.x-3.x     #489      +/-   ##
===========================================
- Coverage    82.14%   81.42%   -0.73%     
===========================================
  Files          243      235       -8     
  Lines         3714     4081     +367     
===========================================
+ Hits          3051     3323     +272     
- Misses         663      758      +95
Impacted Files Coverage Δ
tests/src/Traits/HttpRequestTrait.php 100% <ø> (ø)
...l_core/tests/src/Kernel/Languages/LanguageTest.php 100% <ø> (ø) ⬆️
tests/src/Traits/EnableCliCacheTrait.php 100% <ø> (ø) ⬆️
src/Plugin/GraphQL/TypeSystemPluginManager.php 80% <0%> (ø) ⬆️
src/GraphQL/Buffers/BufferBase.php 89.65% <100%> (ø) ⬆️
tests/src/Kernel/Framework/ResultCacheTest.php 100% <100%> (ø)
src/Plugin/GraphQL/PluggableSchemaBuilder.php 85.71% <100%> (+0.86%) ⬆️
tests/src/Kernel/Extension/RecursiveTypeTest.php 100% <100%> (ø)
tests/src/Kernel/Extension/TypeTest.php 100% <100%> (ø)
tests/src/Kernel/Framework/SchemaCacheTest.php 100% <100%> (ø)
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f28ad9c...9344da5. Read the comment docs.

if ($this instanceof KernelTestBase) {
$this->schemaManagerProphecy = $this->prophesize(SchemaPluginManager::class);

$that = $this;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The prophecy callback is bound to the prophecy object, so $this won't work. But perhaps there's a more elegant way around this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Do they somehow overwrite the normal context (which is "$this from the place you create the closure in") through reflection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It uses Closure::bind. We could just introduce our own callback promise to avoid that.

}

protected function registerSchemaPluginManager(ContainerBuilder $container) {
if ($this instanceof KernelTestBase) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because somebody might attach this to something else than a KernelTest. And PHPStorm gives me autocompletion this way 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

Right but then just use the /** typehint (via comments)?


protected function registerTypeSystemPluginManagers(ContainerBuilder $container) {
if ($this instanceof KernelTestBase) {
$that = $this;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here? Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above

@pmelab pmelab mentioned this pull request Feb 3, 2018
@pmelab
Copy link
Contributor Author

pmelab commented Feb 4, 2018

@fubhy Finally done. The base module tests are all ported too.

@fubhy
Copy link
Contributor

fubhy commented Feb 4, 2018

This looks AMAZING @pmelab

@pmelab pmelab changed the title WIP: Test framework Test framework Feb 4, 2018
@fubhy fubhy merged commit a570e62 into 8.x-3.x Feb 4, 2018
@fubhy fubhy deleted the test-framework branch February 4, 2018 10:15
@wimleers
Copy link

wimleers commented Feb 5, 2018

Nice work!

@pmelab specifically pointed me to tests/src/Kernel/GraphQLTestBase.php, which looks nice and simple. I do have two questions about that class:

  1. Why a kernel test rather than an integration test? It's faster, but it also means you're not testing with actual requests like the client would make them?
  2. Why do responses vary by the user cache context by default? Why not user.permissions?
  protected function defaultCacheContexts() {
    return ['gql', 'languages:language_interface', 'user'];
  }

@pmelab
Copy link
Contributor Author

pmelab commented Feb 5, 2018

@wimleers:

  1. For performance and focus. Parts of the tests issue requests to the HttpKernel, to test the cache subscriber. But the main test interface for extensions is the Processor. They should not bother with the framework.
    Nothing stops us running am set of webtests using a mocked schema though.

  2. Yep, also discovered this while writing tests 😀

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.

3 participants