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

Initial refactor unit test file #1055

Draft
wants to merge 5 commits into
base: development
Choose a base branch
from
Draft

Conversation

jestonihpi
Copy link
Contributor

Description

Create new directory and Renamed test-action.php to ActionTest.php.
Unload WP_UnitTestCase and define it as \WP_UnitTestCase on ActionTest class.

Testing instructions

Screenshots

Checklist:

  • I've tested the code.
  • My code is easy to read, follow, and understand
  • My code follows the accessibility standards.
  • My code has proper inline documentation / docblocks.

Additional Comments

Copy link
Member

@jakejackson1 jakejackson1 left a comment

Choose a reason for hiding this comment

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

A number of the test methods are quite verbose. As part of the refactor we should split these larger tests down into smaller test methods.

@@ -1,14 +1,11 @@
<?php

namespace GFPDF\Tests;
namespace GFPDF\Tests\Controller;
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we should use the same namespace as the class being tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed on recent push.

@@ -475,16 +475,16 @@
},
{
"name": "psr/log",
"version": "1.1.2",
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this whole file from the commit since it's not necessary for your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed on recent push.

@@ -247,7 +245,7 @@ function( $routes ) {
/* Fail capability check */
try {
$this->controller->route();
} catch ( Exception $e ) {
} catch ( \Exception $e ) {
Copy link
Member

Choose a reason for hiding this comment

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

We should look at utilising PHPUnit's @expectException instead of running the test this way. https://phpunit.de/manual/6.5/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.exceptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied and use WPDieException

Copy link
Member

@jakejackson1 jakejackson1 left a comment

Choose a reason for hiding this comment

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

Just a quick review right now. Will do a full one tomorrow.

.gitignore Outdated
@@ -43,6 +43,7 @@ dist/*
_notes
_notes/*
Thumbs.db
composer.lock
Copy link
Member

Choose a reason for hiding this comment

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

The lock file is important to ensure the built copy of Gravity PDF exactly matches what we run in development. My previous comment about not including it in your PR was because it was unrelated to the work you are doing. We try keep our PRs to one specific focus. In this case, refactoring the unit tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted Jake.

$_POST['gfpdf_action'] = 'gfpdf_test_action';

$this->controller->route();

Copy link
Member

Choose a reason for hiding this comment

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

New lines here that aren't 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.

Fixed.

@jakejackson1 jakejackson1 marked this pull request as draft March 3, 2021 01:08
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