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

[thomasplevy]

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:

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

In lifterlms/lifterlms-tests:

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.

[thomasplevy]

@gocodebox/engineering please weigh in <3

[eri-trabiccolo]

@thomasplevy I don't see why we'd need the jetpack class (and don't see why we would use their autoloader as well, to use the latest version of that library? who cares about the latest version :D), I think we can just go with the simple alternative you proposed.

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

if ( ! function_exists( 'llms_defined' ) ) {
  function llms_defined( $name ) {
     return defined( $name );
  }
}

overridden in the test suite as you outlined.

[pondermatic]

I don't normally like adding yet another dependency, but I do like how Automattic/jetpack-constants works.

In the LifterLMS core, use the functions suggested by @thomasplevy and @eri-trabiccolo:

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

if ( ! function_exists( 'llms_defined' ) ) {
    function llms_defined( $name ) {
        return defined( $name );
    }
}

In lifterlms/lifterlms-tests:

function llms_constant( $name ) {
    return Automattic\Jetpack\Constants::get_constant( 'CONSTANT' );
}

function llms_defined( $name ) {
    return Automattic\Jetpack\Constants::is_defined( 'CONSTANT' );
}

# Setup test constants.
function setUp() {
    Automattic\Jetpack\Constants::set_constant( 'CONSTANT', $value );
}

# Teardown test constants.
function tearDown() {
     Automattic\Jetpack\Constants::clear_constants();
}

[thomasplevy]

and don't see why we would use their autoloader as well, to use the latest version of that library? who cares about the latest version :D

Well, if we were to deploy the class to production it would help avoid collisions with other plugins (WooCommerce and Jetpack) in addition to ensuring the latest version is loaded.

---

I like @pondermatic's idea of using Jetpack when running tests only... although honestly the footprint of the constant file is small I'm not sure we really gain much by adding the dependency if we're writing code into the test lib anyway...

[eri-trabiccolo]

and don't see why we would use their autoloader as well, to use the latest version of that library? who cares about the latest version :D

Well, if we were to deploy the class to production it would help avoid collisions with other plugins (WooCommerce and Jetpack) in addition to ensuring the latest version is loaded.

Yeah I get the implications :) I meant that I don't think we needed that b/c 1) collisions could be avoided as we always do with a check on the class existence. No? 2) we don't care about the lib being at the latest version, we actually would care about it being at the version we're sure our code works with. Which I understand can be always the latest if we automatically test against it - and update it in our package. I meant: is this worthwhile given our goal here? My theory is that maybe it's not :).

[thomasplevy]

I agree