BBMSK-16 - Add category lookup to public get function

[thatisrich]

Description

BBMSK-16 - Due to the structural changes to the data stored in the options table to allow top level categories, the methods within the Rest_Api class need updating to include the category level lookup. This PR adds the category option to both the get_value and get_attributes methods and, as a result, the get_option_item private method.

Included in this PR is also some roughed out PHP Unit tests. Currently, these tests mimic the get_value and get_attributes methods and don't use them directly. This will be something that needs building on.

Change Log

• inclusion of phpUnit tests within composer
• addition of phpunit.xml.dist
• change to .gitignore
• addition of .deployignore file
• addition of tests folder structure and class testing files
• updates to class-rest-api.php as outlined in the description above

Steps to test

Unit tests

• ensure phpUnit is installed by running composer install
• run composer run phpunit in the terminal to run the unit tests
• all tests should pass with the result OK (3 tests, 7 assertions)
• in class-rest-api-test.php, change one of the test values (for example $expected_category = 'analytics'; to $expected_category = 'seo'; within the test_get_attributes method)
• re-run the unit tests with composer run phpunit
• note that a test will now fail

Passing Categories into Methods

• ensure the wp_options table within your testing environment contains data
• in plugin.php, call the Rest_Api methods and pass in a category and ID like so:

echo '<pre>';
var_dump( Rest_Api::get_value( 'general', 'ec66c3c8-d512-4ba2-94a4-2b5b3b96980d' ) );
echo '</pre>';

echo '<pre>';
var_dump( Rest_Api::get_attributes( 'general', 'ec66c3c8-d512-4ba2-94a4-2b5b3b96980d' ) );
echo '</pre>';

• load the website front end and notice the data dumped at the top of the page
• if the returned data is NULL, the category or ID does not exist in the data
• if the returned data is a string (for get_value) or an array (for get_attributes), the category and ID are present

Screenshots/Videos

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules

[dom-waterson]

Pulled down and tested. All working, nice work. I'm successfully getting the value and attributes out from the DB. You have some conflicting files. As for the PHP test setup files maybe @markgoodyear could have a look at it?

[ampersarnie]

Hey @thatisrich, Apologies but this is going to be a long one as I'm trying to cover a lot here. Sorry if I'm making any assumptions on what you know/don't know. I've had a look through the Unit tests you've written and I think we need to evaluate what you're attempting here. There's a few things wrong in this case but there's one thing that needs addressing the most here.

The tests you've written aren't actually testing against the Rest_Api class. What you've ended up with are tests that are only testing against the dummy data you've provided in the test class, so we're not actually achieving any benefit from the tests.

Couple of other issues/points

1. For setting dummy data, you can use Data Providers to help structure your test data. I don't think you'll need it in the end for the tests you'll require for this, though.
2. You don't need to assert that the test data is in the format you are using. See this line. You are setting the data so we know its that format. This just adds unnecessary assertions.
3. Logic in tests should be avoided as you're wanting to test the data resulting from class methods and what the data is, not what it could be. See this line. You're also introducing potential points of failure that wouldn't be of the fault of the code you're testing against, so it may cause false-flags.
4. You don't need the PHPUnit_Operational_Test cases as well, as these aren't doing much and provide no benefit to the end result and passing rate of the code.

What should we be testing?

On a high level when it comes to unit testing, we should be testing against a unit of data. This will often be the returned data from a function and/or method in a given class. So what we're looking for is if a class method returns the data we expect it to return. Take this example:

class Example {
	public function multiplyByTwo($num) {
		return $num * 2;
	}
}

With this class we've got a method to multiply a given number so our test assertions would be looking for that returned value:

$example = new Example;
$this→assertEquals(10, $example→multiplyByTwo(5)); // Success!

We're testing that the value returned here is a multiple of 5, which is 10 giving us a successful result. And because this "unit" has a single responsibility we don't need to worry about anything else.

Then this can be expanded on to include various other assertions to ensure we're getting the desired results:

$example = new Example;
$this→assertEquals(10, $example→multiplyByTwo(5)); // Success!
$this→assertIsInt($example→multiplyByTwo(5)); // Success!

We should also be testing what happens if things go wrong. If we use the above example again, we're defining that we're expecting an integer as the $num param, so we can take the opportunity to test against what would happen if we provided a string. This would in turn allow us to identify next steps on how we can handle these issues and refactor some code. This may bring us into TDD territory but I wouldn't worry too much about that at the moment.

So lets take our multiplication example again with these tests:

$example = new Example;
$this→assertEquals(10, $example→multiplyByTwo(5)); // Success!
$this→assertIsInt($example→multiplyByTwo(5)); // Success!
$this→assertEquals(10, $example→multiplyByTwo('five')); // Failure!

Passing the string five would result in the following error:

Fatal error: Uncaught TypeError: Unsupported operand types: string * int

Now we know it needs to be handled as we're getting errors, and a string will cause that. As we know we can do by changing the method like this to not only better protect the code we actually want, but to handle a situation where we're passing through something unexpected:

class Example {
	public function multiplyByTwo($num) {
		if ( is_int($num) ) {
			return $num * 2;
		}

		return false;
	}
}

And we can change our tests to look like:

$example = new Example;
$this→assertEquals(10, $example→multiplyByTwo(5)); // Success!
$this→assertIsInt($example→multiplyByTwo(5)); // Success!
$this→assertFalse($example→multiplyByTwo('five')); // Success!

Now have three assertions that cover two aspects that we should try to cover:

1. What happens when we give it the correct data, and
2. what happens when we give it the incorrect data.

This can all be applied within the same context of WordPress as well as we only need to be testing against that "unit" (or method) and less of the wider scope - Integration tests are a different matter which I wont cover here as there's a difference between Unit and Integration.

However, this comes with its own caveats as WordPress is providing a lot of extra functionality that we rely on. Fortunately there's tools available for that - function and method mocking. To do this we can lean on "Brain Monkey" which provides utilities for us to mock and stub WordPress functions.

With mocking we can create functions that would not normally exist in a test run and give them any return value we need. For example:

class Example {
	public function get_general_options() {
		$all_options = get_options( 'site_settings' );

		return $all_options['general'];
	}
}

As the above is using the get_options() function that comes with WordPress, we need to stub it with some return values for testing. We can do this by leaning on Brain Monkey:

use Brain\Monkey\Functions;

class ExampleTest extends TestCase {
	public function setUp() {
		Functions\Stubs(
			[
				'get_options' => function() {
					return [
						'general' => true,
					];
				}
			]
		);
	}

	public function test_get_options() {
		$example = new Example;
		// Check that the value of our "general" option item is true.
		$this→assertTrue($example→get_general_options()); // Success!
	}
}

What you've effectively done here is told the test to say "get_options doesn't really exist, but when you find code that declares it, pretend it does, and return this array".

There's plenty of other things to look out for but this should give you a good base to start with the tests you're aiming to do. So to summerise:

1. You should be testing the functions/methods and what they're return.
2. You should consider what happens if something fails, not just when its successful.
3. Dummy data is helpful, but only when its there as guidance to facilitate point one, if its needed.

Some resources

Here's a few resources that might help with this:

1. PHPUnit Manual- there's some great information and examples of assertions and how to organise tests.
2. "Brain Monkey" - worth exploring and reading as much as possible as this will help greatly with the context of WordPress.
3. Overview of what to write Similar to what I've done above, gives some examples of testing.
4. Beginners guide to writing tests - another one similar to above.
5. An overview of TDD (Test Driven Development) - thought this might interest with some further reading.

[thatisrich]

Thanks for the awesome feedback and info @ampersarnie, very much appreciated!

As you may have guessed this was my first foray into unit testing so, in hindsight, this should probably have been split across 2 PRs; one to add the category level into the methods and another to handle the unit testing.

I'll dig into the links you've provided and update the testing.

[ampersarnie]

Nice work putting this together @thatisrich, the tests you have now are a massive improvement. Left a couple additional suggestions and considerations for you.

[ampersarnie]

Some things to consider here when writing tests that will impact your code and how you handle misuse or unexpected issues with your method. I'd add additional number formats to this test to ensure you're handling them as expected. Such as:

• Integers (1234567890)
• Floating Point Numbers (1234567890.123)

Adding both of those to the tests will show they return null, which isn't bad. This also allows for some self-documentation in the code to say "yes its an id, but it must be a string" which will always help, and ensures your testing against number variants.

However, that may leave some open questions for yourself:

• In some cases it could be argued that a integer should also be acceptable in the case of an id this way as its expectation is numerical. So an integer failing may be incorrect. So, if you were to accept an integer being used, how is this handled in the code to ensure it passes?
• Are the types in the method docblock for the $id @param correct? (hint: PHPStan also offers advanced string types.)

[thatisrich]

Good points, especially as there currently isn't any specific tests on what $id value is passed in, it just coincidently ends up as a null return as the value's ID will never match the invalid $id param!

Thinking about it, in this case, the ID of a setting will always be a string as it's created using uuidv4() which will follow this structure '1b9d6bcd-bbfd-4b2d-9b5d-ab8dfbbd4bed'. Due to this, an int or a float value will be invalid and never found in the data, so should return null.

To return as soon as possible to optimise the code, I've added a conditional return to the get_option_item() method to check if the $id is a string.

if ( ! is_string( $id ) ) {
	return null;
}

I've also expanded on the tests to cover this approach.

Whilst I was doing this, I noticed I hadn't defined the types in the methods, just in their docblock. Making the change from

public static function get_value($category, $id ): mixed {
    ...
}

to

public static function get_value( string $category, string $id ): mixed {
    ...
}

now breaks the tests that pass in anything other than a string.

This got me thinking further, is it better to be strict with what is passed into the method and not write extra tests as a result, or to be more flexible and have more tests?

[ampersarnie]

I'd go through the same process for number variants in my other comment.

[ampersarnie]

Sorry its taken so long to come back to this. The changes are great, nice work.