Add tests for Config Store API function: loadConfigFromFilesystem

[rgadon107]

@hellofromtonya Hi Tonya! During spare moments this past week, I sat down and worked a bit on the project. I reviewed your comments for PR #20 and merged the development branch into my local version of add/loadConfigFromFS).

My latest set of commits address edge case #6;

With a valid path to the config file, a configuration array that contains a key paired to an empty or null value, and an array of defaults, I expect the API function loadConfigFromFilesystem to provide a $store_key and a $config assigned a value of false. The internal API function _the_store() would throw an Exception error.

Well, I changed the first conditional in _the_store() to return an Exception rather than an empty array stored in $config_store.

Something interesting happened when I attempted to log in and visit the site Admin page. _the_store() threw an Exception! According to the stack track presented by Whoops!, when the site starts up and calls the API function getAllKeysStartingWith( $starts_with ), there's no meta_key configuration stored in _the_store. So instead of returning an empty array, we're now throwing an Exception.

The Exception can be avoided by adding an ! is_admin conditional tag to the first conditional in _the_store(). While that will display the site Admin, the unit tests for _theStore no longer pass. ;-(

Your thoughts?

[hellofromtonya]

Something interesting happened when I attempted to log in and visit the site Admin page. _the_store() threw an Exception! According to the stack track presented by Whoops!, when the site starts up and calls the API function getAllKeysStartingWith( $starts_with ), there's no meta_key configuration stored in _the_store. So instead of returning an empty array, we're now throwing an Exception.

The Exception can be avoided by adding an ! is_admin conditional tag to the first conditional in _the_store(). While that will display the site Admin, the unit tests for _theStore no longer pass. ;-(

Your thoughts?

The store is generic and doesn't have anything to do with WordPress. You can use it on any PHP project. Keeping it decoupled from WordPress is a good strategy.

What do you do instead? Look at the WordPress-specific code to find the root cause. Why is something trying to get a configuration that doesn't exist? Fix the problem there and no in the store itself.

[rgadon107]

@hellofromtonya WordPress fires a hook do_action( ‘admin_menu’, ‘’ );. The callback spiralWebDB\Metadata\register_meta_boxes is registered to that event in wp-content/mu-plugins/central-hub/src/meta-data/meta-box.php. That gets the ball rolling by calling a bunch of API functions that finally ends by invoking _the_store(). [See the following stack trace below.]

The simplest and earliest way that I can intervene to prevent the invocation of the _the_store() is by adding a conditional check for is_admin() within register_meta_boxes() and returning early if true.

function register_meta_boxes() {
if ( is_admin() ) {
return;
}

// rest of the function…
}

While that allows the /wp-admin page to render, it also strips out the custom meta-boxes registered for each custom plugin. Not a useful solution.

What do you do instead? Look at the WordPress-specific code to find the root cause. Why is something trying to get a configuration that doesn't exist? Fix the problem there and not in the store itself.

Since _the_store() is called early when registering the meta_boxes, is it necessary to throw an Exception from _the_store() if the $store_key is empty? I'm wondering here whether the simplest thing to do is to do nothing (not change the first conditional in _the_store()).

If that's not a useful approach, can you suggest another way to approach the problem?

[hellofromtonya]

@rgadon107 There are a couple of strategies to consider:

1. If the key doesn't exist in the store, return an empty array.
2. Or before asking for a key in the store, check first if that key exists. How? Add a has_in_config_store( $store_key ) function to the API.

Which strategy should you select? Start by asking yourself a question:

Is requesting a config that doesn't exist, is that really a cause for a fatal error? Or should the store alert that the config does not exist by returning an empty?

Think about it.

[rgadon107]

@hellofromtonya Working at the front end of the call stack (where the function register_meta_boxes() is hooked into the event admin_menu), we store a config by invoking the API function configStore\getConfigParameter. That function sets a config with a call to the API function getConfig( $store_key). That function checks whether the $store_key is empty or not. If the $store_key is empty, then it returns an empty array. Otherwise, return _the_store() keyed by the $store_key.

I don’t think it’s necessary to add an API function has_in_config_store( $store_key) as a check on the front end of the call stack. That functionality is already done for us in the API function getConfig(). What I found is that I can change the conditional check in getConfig() and get the local site and the meta-boxes to render. I changed the function to this:

function getConfig( $store_key ) {
if ( empty( $store_key ) ) {
throw new \Exception(
sprintf(
'The store key does not exist within the configuration to store.' )
);
}
}

Then in wp-content/mu-plugins/central-hub/src/config-store/internals.php, we can allow _the_store() to return an empty array if the $store_key is empty.

What do you think of this approach?

The next step would be to write a passing unit test for getConfig() to throw the exception in the event that the $store_key is empty.

[hellofromtonya]

@rgadon107 No code except the API should be calling _the_store(). Rather, the code should be calling getConfig(). Therefore, moving the error message does not resolve the issue.

Instead, consider returning something, but not throwing an error.

Let's walk through a thought experiment:

1. Something like the meta API requests a configuration. If it exists, it receives an array with the configuration parameters. Else, it returns nothing (which could be an empty array, false, null, etc.).
2. Think about how the requester would have receiving nothing back. It should have a check and then respond accordingly. In the case of the meta API, it could bail out.

What do you think?

[rgadon107]

@hellofromtonya This makes sense. Since we already have a 'guardrail' condition inside getConfig to check whether a value is stored in the parameter ($store_key), we'll rely on that. If no value is stored, then return an empty array. If a value is stored, then return _the_store().

We don't touch _the_store() or the API function getConfig. I'll have to go back and check, but I think that would resolve edge case #6 for the API function loadConfigFromFilesystem.

[hellofromtonya]

@rgadon107 Almost there. Good job. I'd go further though.

1. If the store key doesn't exist in the store, just return an empty value, such as an empty array.
2. That means: don't throw an error in the _the_store() if the store key doesn't exist.

[rgadon107]

@hellofromtonya Yes, I agree. The first guardrail condition in _the_store() checks the value of the $store_key. If it's empty, then it will return the value of $config_store which is an empty array. PHP defines the term empty as:

A variable is considered empty if it does not exist or if its value equals FALSE. empty() does not generate a warning if the variable does not exist.

Values regarded as empty include:

• "" (an empty string)
• 0 (0 as an integer)
• 0.0 (0 as a float)
• "0" (0 as a string)
• NULL
• FALSE
• array() (an empty array)

There will be no Exception thrown in _the_store in the event that the $store_key is an empty value.

I will go back through the commits on this branch and review the changes I submitted for your review. I'll probably just revert the branch to an earlier commit and drop those changes. Then we can move forward with the other edge cases for loadConfigFromFilesystem.

[rgadon107]

@hellofromtonya Hi Tonya, I wrote 4 tests today for edge cases #1 - 4 for loadConfigFromFilesystem. Got the first 3 tests to work. I'm stuck on edge case #4:

With a valid path to the config file, a valid configuration array, an array of defaults, and an existing configuration stored in _the_store(), I expect the function to overwrite the existing stored configuration, and return a new store_key.

[Edge case #4]... will not return a new store key. Rather, it returns its store key, i.e. the one identified in the file’s configuration array.

I'm using BM's expect() method to mock a config already stored in _the_store.

My test returns the following error message:

Mockery\Exception\InvalidCountException: Method KnowTheCode_ConfigStore__the_store('baz', 'config_to_store') from Mockery_0 should be called exactly 1 times but called 0 times.

Can you help me to think through what I'm doing wrong?

=================================================

Update: I was using BM's expect() method when I should have been using when(). I went back to the Unit Testing with Brain Monkey lab and reviewed Episode 10 ( expect() method ). I found my error.

I need to go back and listen again to the episodes on the use of when() and expect() to distinguish the different applications of each method.

This branch is ready for CR and merge with development.

[hellofromtonya]

@rgadon107 I just pushed commits to fix the unit tests and add integration tests for this API function. A few notes for you:

1. Notice how we use mocks to check each dependency, even if we expect it to never be invoked.
2. I removed the error for invalid path, as that's a PHP error for require. We don't need to test PHP internal functions.
3. The integration tests pull it all together by exercising the internal dependencies against what we expect to happen.

Look the tests over. Ask questions. When you're ready, flag me and I'll merge this one.

[hellofromtonya]

@rgadon107 Notice that we are invoking this 2x.

[hellofromtonya]

@rgadon107 Notice that we are checking that this function does not get invoked.

[hellofromtonya]

@rgadon107 Look at each of the mocked internal function dependencies. We are checking:

1. The number of times each is invoked, i.e. once().
2. The arguments we expect each to received, i.e. with.
3. The value that is returned, i.e. andReturn.

When we invoke loadConfigFromFilesystem(), each of these internal functions will be exercised. BrainMonkey is checking the number of times and arguments for us. It then mocks the function out and returns the value we coded.

[hellofromtonya]

@rgadon107 Notice how this integration test is constructed. What is it doing?

1. It's validating that _load_config_from_filesystem() does do what we expect:
   • Loads the config from file.
   • throws the error we expect
2. _merge_with_defaults does get called.
3. _the_store does get called.

Writing a test in this way makes it very clear as to what behavior we expect and don't expect to happen.

[rgadon107]

@hellofromtonya Well, this CR was surprising and instructive. It's was a useful exercise in the application of BM and distinguished the construction of unit versus integration tests. This branch will be useful going forward to better understand how to think about and construct unit & integration tests.