Explore using PHPUnit for feature tests

[ewhanson]

I am currently exploring with it could look like to make greater use of so-called "feature tests" with PHPUnit to do more of testing. While there can be many different definitions of unit vs. feature tests, for the purpose of this discussion, I'm considering any PHPUnit test that tests overall functionality that may require hitting the database or combining discrete systems to complete an action as feature tests. An example of this would be using PHPUnit tests to ensure publication ordering behaves in an expected way in a number of different circumstances, e.g. added sequentially to an issue, when different version of a submission are created, when custom ordering is used, etc.

This will help in form part of the discussion in #10032.

As I try things out, I will add to the discussion here.

[ewhanson]

2024-06-18 (😖)

Working off the main branch of OJS for now. I have a DatabaseTestCase setup that looks roughly like this to play with:

class ExplorationTest extends DatabaseTestCase
{
    public function getAffectedTables()
    {
        return PKPTestHelper::PKP_TEST_ENTIRE_DB;
    }

    public function testDebug(): void
    {
        Repo::submission()->delete(Repo::submission()->get(1));
        $subCount = DB::table('submissions')->count();
        $this->assertNotEquals(18, $subCount);
    }

    public function testDebugTwo(): void
    {
        $subCount = DB::table('submissions')->count();
        $this->assertEquals(19, $subCount);
    }
}

Fixable errors (through documentation, user configuration, etc.):

• PKPTestHelper::restoreDB() uses zcat utility, which doesn't behave as expected on Mac. Had to follow directions in this Stack Exchange post. Short answer: GNU versions of tools are needed. Had to run brew install coreutils gnu-sed, which installs GNU versions of sed and zcat. Also had to alias sed and zcat to their respective GNU versions, e.g. alias sed=gsed, etc.

Errors requiring code changes:

• PKPTestHelper::restoreDB() also assumes the location of mysql to be /usr/bin/mysql which may not be the case (it is not the case on ARM-based Macs using homebrew, which will instead be /opt/homebrew/bin/mysql). I imagine this may also be an issue on Windows.
• I wanted to test the database resetting by having two tests that assert the count of submissions in the database. I was working off a copy of the generated dataset and wanted to delete a submission to make sure it was restored when the next test was run. I tried to do so using something like Repo::submission()->delete($submission). This worked up until Repo::submission()->updateStatus() attempts to get the context from the request using $this->request->getContext(). This will cause the test to fail because PKPRequest::_delegateToRouter() (see: assert false statement) will assert(false) since there is no request in this testing context. This isn't helpful in this particular context as it immediate fails the process, throwing the exception up to PHP Unit, but the method itself is fine if there's no request context (it instead gets the context it needs from the submission's context). It might not be okay in all cases, but if this assert(false) were not there, it wouldn't break the test (as this is accounted for). To try and capture full historical context, the assert(false) was added in 2011 by Bruo Beghelli as a single line commit.

Next steps:

• Try testing without PKPRequest::_delegateToRouter() asserting false when it's made from a CLI context (or really any non-web request context).
• Think about overall need for/use of the web request in what should be runtime environment-agnostic applications (like Repo functionality).

[asmecher]

Some things that bug me about the current DatabaseTestCase approach (aside from its nuke-it-from-space level of subtlety):

• It doesn't play especially well with the datasets repo, which most devs will be using to provide database images. (It requires a DATABASEDUMP environment variable to tell it where a .sql.gz will live, rather than using the tools/conventions that live in datasets; the datasets database dumps aren't gzipped; it reimplements the database-specific implementations, which are also provided in datasets.)
• It encourages us to code tests to the test dataset, whereas it might be better to have each test set up / tear down its own data as needed, in an encapsulated form. (Fundamentally our tests run too slowly; the urge to re-use existing test content probably stems from trying to avoid that problem rather than solving it.)
• It's likely to lead to quirky problems with caching, jobs, integrations, and other cases where an external system doesn't expect a brute-force rollback.

To be clear, I'm not 100% against this approach, just want to document some of its current issues!