Improving unit tests coverage #49

ezimuel · 2018-09-24T15:34:56Z

This PR improves the unit tests coverage.

weierophinney

First off, great work - you jumped from 27% to 85% coverage!

There's still some room for improvement, though the improvements I suggest may not necessarily add coverage; they'll mainly make intent of tests more clear. In broad strokes:

Testing constructors to ensure they create an instance of the class is mostly useless, unless you also test for default property values.
Rename the various test methods to describe the behavior being tested. This will make the intent more clear, and may help you identify when you need to split a test into multiple tests and/or use a provider.

weierophinney · 2018-09-25T13:01:46Z

src/ConfigTrait.php

-        if (empty($config['grants']) || ! is_array($config['grants'])) {
+        if (empty($config['grants'])) {
            throw new InvalidConfigException(
                'The grant value is missing in config authentication'


This should be "grants"; otherwise, users might add a grant configuration value, and still get the exception. I would also likely add a note that the value must be an array.

Ok, I changed in "grants" and added "must be an array" in the exception message.

weierophinney · 2018-09-25T13:01:57Z

src/ConfigTrait.php

        }
+        if (! is_array($config['grants'])) {
+            throw new InvalidConfigException(
+                'The grant must be an array value'


Same comment here.

weierophinney · 2018-09-25T13:04:33Z

test/ConfigTraitTest.php

+    /**
+     * @expectedException Zend\Expressive\Authentication\OAuth2\Exception\InvalidConfigException
+     */
+    public function testGetPrivateKeyNoConfig()


Update your test names to indicate what behavior is being tested. In this case, the method would become something like testGetPrivateKeyWhenNoConfigPresentWillResultInAnException.

This practice makes understanding the intention behind the test clearer when maintainers are trying to understand a failure later.

Ok, I supposed was clear enough :)

weierophinney · 2018-09-25T13:05:19Z

test/ConfigTraitTest.php

+<?php
+/**
+ * @see       https://github.com/zendframework/zend-expressive-authentication-oauth2 for the canonical source repository
+ * @copyright Copyright (c) 2017 Zend Technologies USA Inc. (https://www.zend.com)


Here and elsewhere you introduce new test classes: since the file was introduced this year, use 2018 as the copyright year.

weierophinney · 2018-09-25T13:06:58Z

test/Entity/AccessTokenEntityTest.php

+use PHPUnit\Framework\TestCase;
+use Zend\Expressive\Authentication\OAuth2\Entity\AccessTokenEntity;
+
+class AccessTokenEntityTest extends TestCase


I don't think we need this test. If it's just testing that the constructor returns an instance of the class, that's something we know already.

If there's other behavior to test — e.g., default property values, property values based on constructor arguments, etc. — then tests make sense.

If I remove the AccessTokenEntityTest we need to use @codeCoverageIgnore to omit this class from the code coverage. IMHO I think a test class like this, even if just checking for constructor, can be useful as a constrain for future changes. For instance, if someone changes the AccessTokenEntityInterface this test is able to intercept it.

weierophinney · 2018-09-25T13:08:43Z

test/Entity/RefreshTokenEntityTest.php

+
+class RefreshTokenEntityTest extends TestCase
+{
+    public function testConstructor()


Same comment here as for the AccessTokenTest.

I renamed to testImplementsRefreshTokenEntityInterface to test the interface implementation, not just the constructor.

weierophinney · 2018-09-25T13:09:28Z

test/Entity/RevokableTraitTest.php

+
+class RevokableTraitTest extends TestCase
+{
+    public function testRevoked()


Break this into two tests, or use a test provider, to detail what happens with each of the boolean arguments.

weierophinney · 2018-09-25T13:09:46Z

test/Entity/ScopeEntityTest.php

+        $this->entity = new ScopeEntity();
+    }
+
+    public function testConstructor()


This method is unnecessary.

I renamed into testImplementsScopeEntityInterface.

weierophinney · 2018-09-25T13:10:51Z

test/Entity/TimestampableTraitTest.php

+
+namespace ZendTest\Expressive\Authentication\OAuth2\Entity;
+
+use DateTime;


Quick question: should we be using DateTime, or DateTimeImmutable? Which would make more sense for the domain?

I would say accept DateTimeInterface, convert to DateTimeImmutable if mutable.

I used DateTimeInterface as parameters of TimestampableTrait and DateTimeImmutable in TimestampableTrait::timestampOnCreate() function.

weierophinney · 2018-09-25T13:11:32Z

test/Entity/UserEntityTest.php

+    /**
+     * @expectedException ArgumentCountError
+     */
+    public function testConstructorWithoutParam()


This test doesn't have any assertions. Assert against the default identifier.

The assertion is in the phpdoc, I rename the function to testConstructorWithoutParamWillResultInAnException.

Xerkus · 2018-09-25T13:37:07Z

I would suggest explicit class level @covers annotations with forceCoversAnnotation="true" phpunit option to avoid false/accidental coverage

ezimuel · 2018-09-26T13:14:31Z

@weierophinney I updated the PR including your feedback.

- Commas after all array entries - When splitting to multiple lines, move all arrow operations to their own lines when operating on an object

Writes three test methods that ensure the arguments passed to the constructor affect the instance state.

s/testJsonSerialize/testEntityIsJsonSerializable/

- New files: 2018 - Existing files: range adds "-2018"

So we can test against PHP 7.3.

Close #49

ezimuel added 2 commits September 21, 2018 17:04

WIP: improving unit tests

1911c3d

Improve unit tests completed

a03545a

ezimuel requested a review from weierophinney September 24, 2018 15:35

weierophinney suggested changes Sep 25, 2018

View reviewed changes

Resolved comments on PR zendframework#49

bba9b21

weierophinney added 6 commits September 26, 2018 11:19

Minor CS/consistency changes

458d26a

- Commas after all array entries - When splitting to multiple lines, move all arrow operations to their own lines when operating on an object

Write tests for ClientEntity constructor

c569c1c

Writes three test methods that ensure the arguments passed to the constructor affect the instance state.

Fixes typo in method name of RevokableTraitTest

c641138

Better test method name in ScopeEntityTest

3c4a356

s/testJsonSerialize/testEntityIsJsonSerializable/

One line per object operation

0a83f12

Updates copyright dates on all files touched by this patch

cc0a998

- New files: 2018 - Existing files: range adds "-2018"

weierophinney approved these changes Sep 26, 2018

View reviewed changes

Adds php-nightly build to travis

7acf6b8

So we can test against PHP 7.3.

weierophinney merged commit 7acf6b8 into zendframework:master Sep 26, 2018

weierophinney added a commit that referenced this pull request Sep 26, 2018

Merge branch 'feature/49-test-coverage'

14edccd

Close #49


		namespace ZendTest\Expressive\Authentication\OAuth2\Entity;

		use DateTime;

Improving unit tests coverage #49

Improving unit tests coverage #49

Uh oh!

Conversation

ezimuel commented Sep 24, 2018

Uh oh!

weierophinney left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xerkus commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ezimuel commented Sep 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Xerkus commented Sep 25, 2018 •

edited

Loading