Conversation
jakejackson1
left a comment
There was a problem hiding this comment.
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.
| <?php | ||
|
|
||
| namespace GFPDF\Tests; | ||
| namespace GFPDF\Tests\Controller; |
There was a problem hiding this comment.
Ideally, we should use the same namespace as the class being tested.
There was a problem hiding this comment.
Fixed on recent push.
| }, | ||
| { | ||
| "name": "psr/log", | ||
| "version": "1.1.2", |
There was a problem hiding this comment.
Let's remove this whole file from the commit since it's not necessary for your PR.
There was a problem hiding this comment.
Fixed on recent push.
| try { | ||
| $this->controller->route(); | ||
| } catch ( Exception $e ) { | ||
| } catch ( \Exception $e ) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Applied and use WPDieException
…factor on verbose methods
jakejackson1
left a comment
There was a problem hiding this comment.
Just a quick review right now. Will do a full one tomorrow.
.gitignore
Outdated
| _notes | ||
| _notes/* | ||
| Thumbs.db | ||
| composer.lock |
There was a problem hiding this comment.
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.
| $_POST['gfpdf_action'] = 'gfpdf_test_action'; | ||
|
|
||
| $this->controller->route(); | ||
|
|
There was a problem hiding this comment.
New lines here that aren't needed.
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:
Additional Comments