LibriVox / librivox-catalog

LibriVox catalog and reader workflow application
https://librivox.org
MIT License
36 stars 17 forks source link

Add ci-phpunit-test using composer #190

Closed garethsime closed 4 months ago

garethsime commented 4 months ago

I've been playing around with how to get started writing tests for Librivox for a little while. ci-phpunit-test came up in a lot of my searches, so I gave it a go, and I thought I'd share what I learned and propose that it gets added to librivox-catalog.

So far, it's been pretty good. It was pretty easy to install (both manually and via composer, I don't know if you have a preference). I've used it to write tests while working on a couple of PRs, and I've included some of them below.

One thing to be wary of is that it looks like the project hasn't had a lot of activity lately, but I think that's going to be the case with pretty much any CodeIgniter 3.x library. (The library maintainer's recommendation is that we upgrade to CodeIgniter 4, but that looks like it might be a big piece of work and I don't know what plans there are for it, if any.)

Note: Most of the files here are boilerplate/config and were generated by following the install instructions.

Example from pull-request #186

Here's an example from checking that accessing /author/0 returns a 404 and that searching for authors using primary_key=0 returns a 400:

class Author_test extends TestCase
{
    public function test_zeroth_author_returns_404() {
        $this->request('GET', 'author/0');
        $this->assertResponseCode(404);
    }

    public function test_search_without_author_id_returns_400() {
        $this->request(
            'GET',
            'author/get_results',
            [
                'primary_key' => 0,
                'search_page' => 1,
                'search_order' => 'alpha',
                'project_type' => 'either',
            ]
        );
        $this->assertResponseCode(400);
    }
}

Example from pull-request #189

And here's an example test that I wrote to check that we were normalising and squashing down newlines correctly:

<?php

class description_html_render_helper_test extends TestCase
{
    public function setUp(): void
    {
        $this->resetInstance();
        $this->CI->load->helper('description_html_render_helper');
    }

    /**
         * @dataProvider provider
         */
    public function test_newline_smushing_works($input, $expected) {
        $actual = _normalize_and_deduplicate_newlines_in_html($input);
        $this->assertEquals($expected, $actual);
    }

    public static function provider() {
        return array(
            // Simple case
            array("a b", "a b"),

            /** A bunch of cases omitted for brevity **/

            // Mixed "newline" things
            array("a<br>\n<br />b",       "a<br /><br />b"),
            array("a<br>\r\n<br />b",     "a<br /><br />b"),
            array("a<br>\n\r\n<br />b",   "a<br /><br />b"),
            array("a\n<br />\r\n<br />b", "a<br /><br />b"),
            array("a\n\r\n<br />b",       "a<br /><br />b"),
        );
    }
}

Some sample output when the tests pass:

/librivox/www/librivox.org/catalog# XDEBUG_MODE=coverage ./vendor/bin/phpunit -c application/tests/
PHPUnit 10.5.10 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.2-1ubuntu2.14 with Xdebug 3.1.2
Configuration: /librivox/www/librivox.org/catalog/application/tests/phpunit.xml

.......................                                           23 / 23 (100%)

Time: 00:00.398, Memory: 14.00 MB

OK (23 tests, 23 assertions)

Generating code coverage report in Clover XML format ... done [00:00.376]

Generating code coverage report in HTML format ... done [00:01.990]

Some sample output when the tests fail:

/librivox/www/librivox.org/catalog# XDEBUG_MODE=coverage ./vendor/bin/phpunit -c application/tests/
PHPUnit 10.5.10 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.2-1ubuntu2.14 with Xdebug 3.1.2
Configuration: /librivox/www/librivox.org/catalog/application/tests/phpunit.xml

.................F..FF.                                           23 / 23 (100%)

Time: 00:00.417, Memory: 14.00 MB

There were 3 failures:

1) description_html_render_helper_test::test_newline_smushing_works_parameter with data set #13 ('a<br><br><br><br>b', 'a<br /><br />b')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'a<br /><br />b'
+'a<br /><br /><br />b'

/librivox/www/librivox.org/catalog/application/tests/helpers/description_html_render_helper_test.php:16
/librivox/www/librivox.org/catalog/vendor/bin/phpunit:122

2) description_html_render_helper_test::test_newline_smushing_works_parameter with data set #16 ('a<br>\n\r\n<br />b', 'a<br /><br />b')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'a<br /><br />b'
+'a<br /><br /><br />b'

/librivox/www/librivox.org/catalog/application/tests/helpers/description_html_render_helper_test.php:16
/librivox/www/librivox.org/catalog/vendor/bin/phpunit:122

3) description_html_render_helper_test::test_newline_smushing_works_parameter with data set #17 ('a\n<br />\r\n<br />b', 'a<br /><br />b')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'a<br /><br />b'
+'a<br /><br /><br />b'

/librivox/www/librivox.org/catalog/application/tests/helpers/description_html_render_helper_test.php:16
/librivox/www/librivox.org/catalog/vendor/bin/phpunit:122

FAILURES!
Tests: 23, Assertions: 23, Failures: 3.

Generating code coverage report in Clover XML format ... done [00:00.383]

Generating code coverage report in HTML format ... done [00:01.971]
notartom commented 4 months ago

I've been playing around with how to get started writing tests for Librivox for a little while. ci-phpunit-test came up in a lot of my searches, so I gave it a go, and I thought I'd share what I learned and propose that it gets added to librivox-catalog.

So far, it's been pretty good. It was pretty easy to install (both manually and via composer, I don't know if you have a preference). I've used it to write tests while working on a couple of PRs, and I've included some of them below.

I'm a huge fan of TDD, automated testing, and CI, so I love this :) I wish the code had proper testing from the beginning, but this is a start on retroactively adding tests, which will in turn allow us to change things with more confidence that we aren't breaking something else.

One thing to be wary of is that it looks like the project hasn't had a lot of activity lately, but I think that's going to be the case with pretty much any CodeIgniter 3.x library. (The library maintainer's recommendation is that we upgrade to CodeIgniter 4, but that looks like it might be a big piece of work and I don't know what plans there are for it, if any.)

Moving to CI 4, while not crazy, is not trivial either. The original author of the code base is long gone, so one really knows how all of it works. Just moving to CI3 from CI2 was a whole thing, and that mostly didn't require rewriting stuff. My opinion is that we'll get more value out of adding tests for stuff, than moving to CI4.

Now that this PR is merged, I'd love to see it run in GitHub Actions. The slight wrinkle is that it needs to run in a "deployed" repository, with the configs and database in place, so we can't just check out this repo and run pphunit :( If we can somehow leverage https://github.com/LibriVox/librivox-ansible/blob/master/.github/workflows/deploy-localdev.yaml to get CI on this repo, I'd be really happy.

notartom commented 4 months ago

It took me an embarrassingly long amount of time chasing down the wrong things (GitHub refs) before realizing I was getting bit by Ansible variable precedence, but we can now run unit tests on PRs in CI!

https://github.com/LibriVox/librivox-catalog/pull/191

garethsime commented 4 months ago

Oh wow, that's awesome! Thanks for putting in all the hard work, it looks great 🙂

(Maybe I'll have to learn how LXC works now haha)