gocodebox / gocodebox/lifterlms

Request for Feedback: Abstracting constant getters/setters for easier testing

Open
#1,757 6 comments 0 reactions 1 assignee Claimed by @thomasplevy View on GitHub
Dominant language
PHP
Stars
212
Forks
140
Avg merge
2d 14h
Merged PRs (30d)
19

Description

## The Problem

In order to test functions/methods/etc... which have different behavior dependent on the value of a constant we need to have multiple tests that are tagged to run in a separate PHP process. This is necessary because, well, constants are constants so if you have a boolean condition you can't test them both without running at least one test in a separate process.

This isn't *terribly* problematic. But running tests in a separate process does *slow down the test suite quite a bit*

## Proposed solution

Abstract the native php methods `constant()` and `defined()` and use a utility method to set constants in the test suite.

The folks at Jetpack have already figured this out (it's not terribly complicated but it already exists and we can just use it: https://github.com/Automattic/jetpack-constants

If we decide to use this, we should likely also start using this too: https://github.com/Automattic/jetpack-autoloader (this is designed to prevent collisions in composer packages used by more than a single WP plugin). WooCommerce, for example, uses the constants lib and (presumably Jetpack itself though I haven't confirmed this). The autoloader package ensures that only one version (the latest) version of the library is actually loaded.

The alternative solution would be to create our own version of this in the LifterLMS core which we can mock ourselves in the test suite.

THe only reason I've hesitated is that the Jetpack constants is the same in a distributed plugin and in the test suite. Whenever you read a constant it first checks the abstracted constants array and then fallsback to the actual constant. I think a "preferred" solution would be to have a set of pluggable functions which we can redefine in the test suite. This would allow us to skip checks against abstraction in live sites but in our test suites we can use the abstractions.

For example, in the LifterLMS core:

```php
if ( ! function_exists( 'llms_constant' ) ) {
function llms_constant( $name ) {
return constant( $name );
}
}
```

In lifterlms/lifterlms-tests:

```php
function llms_constant( $name ) {
global $llms_tests_constants;
if ( array_key_exists( $name, $llms_tests_constans, true ) ) {
return $llms_tests_constants[ $name ];
}
return constant( $name );
}

function llms_tests_set_constant( $name, $value ) {
global $llms_tests_constants;
$llms_tests_constants[ $name ] = $value;
}
```

I think we'd want to create pluggable wrappers for `defined()` as well that would work similarly.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.