Skip to content
This repository has been archived by the owner on Oct 16, 2024. It is now read-only.

Add PHPUnit testing framework #181

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Add PHPUnit testing framework #181

wants to merge 1 commit into from

Conversation

nickpagz
Copy link
Contributor

@nickpagz nickpagz commented Jan 18, 2023

Purpose of this PR:

Implement the PHPUnit testing framework on the Team 51 CLI tool to support development.

Approach

This implementation focuses on unit testing. While Symfony includes command level testing, a finer grain approach would produce more valuable outcomes for developers modifying existing and/or creating new commands.

Symfony includes a built-in implementation and integration with PHPUnit to scaffold in the required libraries which was used for this PR.

The intent is to keep testing optional, and as such the tests won't be part of any commit or merge level checks. The tests can be run manually as required.

Changes proposed in this Pull Request:

  • Add the PHPUnit Testing framework via the built-in Symfony symfony/test-pack package through composer.
  • Extend the PHPUnit TextCase class to allow custom methods, factories, test doubles, etc to aid in test development.
  • Include basic sample tests as go-bys for future tests.
  • Add a custom method in the extended class to allow for easier testing of protected and private methods.

How to Test:

  • From your team51-cli folder fetch updates and switch to this branch.
  • Run composer install to install the new packages.
  • Run all sample tests using the command vendor/bin/phpunit --testsuite tests --testdox.

Adding new tests:

  • Tests are sorted into to the same folder structure as the commands. Command tests go in tests/commands/<command-class-name>, and Helper tests in tests/helpers/<helper-class-name>.
  • Test files must be named with the prefix test- to be run automatically via the testsuite.
  • Tests can be built by extending either the default TestCase or the extended T51TestCase if needed.
  • Custom methods, assertions, fixtures, etc should be added in the tests/includes folder.

* @return mixed The return value of the method.
*/

public function invoke_method( &$object, $method_name, array $parameters = array() ) {
Copy link
Member

Choose a reason for hiding this comment

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

How would this work if we wanted to test a static method on a class that wasn't instantiated? Would there be a recommended other method to use, or a way to override this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still on that learning curve, and so far I haven't found a way since we need to pass an object to instantiate the ReflectionClass. Those static methods would ideally be public, at least if we want to test them.
This brings up a greater philosophical question - ie should we even be testing private or protected methods? From what I understand, the answer is generally no. If that's the case then we're going to have a difficult time unit testing the cli tool, and maybe should look into command level testing, though I feel that's not helpful for finding issues in the individual methods within a class/command.
Does this mean we should/could be using public methods instead? (Rhetorical, kind of, opinions welcomed).

Copy link
Member

Choose a reason for hiding this comment

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

Honestly, if it's something we'd like tested, I'm 100% in favor of just making it public.

*
* @return mixed The return value of the method.
*/

Copy link
Member

Choose a reason for hiding this comment

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

Probably don't need this single blank line.

Suggested change

*/
public function test_get_request_url_adds_prefix_with_integer() {
$expected = 'https://public-api.wordpress.com/rest/v1.1/sites/123';
$actual = $this->invoke_method( $this->wpcom_api_helper, 'get_request_url', array( 'sites/123' ) );
Copy link
Member

Choose a reason for hiding this comment

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

How does this differ in practice from just

Suggested change
$actual = $this->invoke_method( $this->wpcom_api_helper, 'get_request_url', array( 'sites/123' ) );
$actual = $this->wpcom_api_helper->get_request_url( 'sites/123' );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would give you an error since that method is private. The invoke method changes that with the $method->setAccessible( true ); in the ReflectionClass.
Almost all of the methods in our commands, and most of the helpers, are protected or private. To make testing of these easier I decided to add the invoke_method as part of the extended TestCase to avoid having to re-write the method every time.

Copy link
Member

Choose a reason for hiding this comment

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

Got it! That makes sense.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants