[9.x] Improve redis test suite

[TheLevti]

This is a follow up of https://github.com/laravel/framework/pull/40569, splitting the original pull request up. This pull request is about refining the existing redis test suite making it easy to test future changes related to redis.

What changed

• Added data providers with named data sets to allow easy testing of different redis connection configurations.
• All tests in all components that use redis are now using the new data providers to test all supported redis connection settings separately making it easy to pinpoint which redis setting causes an issue with an existing or new component.

If any test related to redis fails, it will now output the exact data set that caused the failure:

How to use

Now when you need to test something where redis is used, you can simply add the helper trait in the beginning like so:

use InteractsWithRedis;

Then to let a test run through all possible redis variations, add one of the following doc blocks:

/**
 * Runs this test with only two basic redis connections where one is using the predis driver and
 * the other the phpredis driver.
 *
 * @dataProvider redisConnectionDataProvider
 */
public function testSomethingRelatedToRedis($connection)
{
    $this->app['redis'] = $this->getRedisManager($connection);
    ...
}

/**
 * Runs this test with the basic connection set and in addition advanced connection configurations like
 * persistent connections, prefix, scan options, serialization, compression etc.
 *
 * @dataProvider extendedRedisConnectionDataProvider
 */
public function testSomethingRelatedToRedis($connection)
{
    $this->app['redis'] = $this->getRedisManager($connection);
    ...
}

To get a custom redis connection during a test you can also pass two additional parameters to the getRedisManager helper method where the second parameter is the driver and third parameter an connection config array:

$redisManager = $this->getRedisManager('predis_custom', 'predis', [
    'cluster' => false,
    'options' => [
        'prefix' => 'test_',
    ],
    'default' => [
        'url' => "redis://{$host}:{$port}",
        'database' => 5,
        'timeout' => 0.5,
    ],
]);

To properly clean up after every test provide the following method call in your tearDown method:

protected function tearDown(): void
{
    $this->tearDownRedis();

    parent::tearDown();
}

[TheLevti]

@tillkruss could you have a look as well, please?

[taylorotwell]

I get some Redis failures when running bash bin/test.sh

[TheLevti]

I get some Redis failures when running bash bin/test.sh

Thank you, I will try to run it locally and fix the remaining issue. So far I was just targeting the CI to pass.

[TheLevti]

I get some Redis failures when running bash bin/test.sh

~~Could you maybe tell me which test fails for you locally? I currently only see that the various redis connection tests fail (scheme, sentinel, username/password). But not sure whether this is because I am running the testsuite on a linux distro rather than mac (test.sh was written for mac it seems).~~

~~I will try to resolve those connection issues.~~

Found the issue. The testsuite locally or in CD is not properly set up to allow TLS/SSL/sentinel connections to redis via predis. It's also not the purpose of those tests to actually test the connection, but rather whether the parameters are properly picked up and set on the connection class. After reverting the changes for the connection tests, it works now.

[TheLevti]

@taylorotwell @tillkruss The pull request is ready for the final review. Local test run is fixed (I was not aware of /bin/test.sh before 😅).

The issue was that the local setup is not configured to support actual tls/sentinel connections (new code tries to always connect, flush the db on connect and flush the db on disconnect in the end, which does not work locally for those connections). So I reverted those specific connection tests to just validate whether the connection params are properly set.

[taylorotwell]

Whenever I run the framework tests (current 9.x branch) locally I get normal output. When I run this branch's test locally my output is all garbled and I get weird errors:

[TheLevti]

Whenever I run the framework tests (current 9.x branch) locally I get normal output. When I run this branch's test locally my output is all garbled and I get weird errors:

Hmm that is really strange. I just ran on latest 9.x and on this feature branch and both times tests finished. Note on the side: because the script expects the www-data user to be used, I first had to change the owner of the folders to www-data to make it work otherwise this FilesystemAdapterTests were always failing regardless of the branch.

I am using a linux machine. Can someone else as well try to run the test suite locally for this branch please?

[Huggyduggy]

I've made some adjustments within phpunits config

         cacheResult ="false"
         enforceTimeLimit="true"  (necessary as I don't have dynamodb AWS keys)

    <logging>
    <log type="testdox-text" target="php://stdout"/>
</logging>

And got the following output on W11 + WSL2 (tests running within docker). None of those seem to have any connection to redis tests, but are related to either the test.sh being written with macOS in mind or the docker image itself (webp missing -> gd): pull41718.log

[driesvints]

@TheLevti I see the same thing as Taylor. Works on 9.x but get weird results on your branch.

[TheLevti]

@TheLevti I see the same thing as Taylor. Works on 9.x but get weird results on your branch.

Thanks guys, I will see what I can find. I don't see a reason how it is related to the application code/redis changes.

I have a feeling that we have a bad local test env setup and also as we noticed its tightly coupled to mac users only, which should not be when using docker. Most likely my changes triggered some tests to actually run/write to disk, which did not work properly before. For example for me the suite on my branch only starts to work once I change the owner of all files to www-data (not in the container), which should not be required as we can not expect that everyone has a www-data user.

Going to try and fix this platform inconsistency these days.

[tillkruss]

This works for me on macOS using iTerm2:

gh repo clone laravel/framework
gh pr checkout 41718

vendor/bin/phpunit

But I do get a couple of incomplete tests:

There were 12 incomplete tests:

Illuminate\Tests\Integration\Cache\RedisStoreTest::testItCanAddWithTtl with data set "predis" ('predis')
Needs support in the RedisStore first.

/Users/Till/Development/Repositories/framework/tests/Integration/Cache/RedisStoreTest.php:88

Illuminate\Tests\Integration\Cache\RedisStoreTest::testItCanAddWithTtl with data set "phpredis" ('phpredis')
Needs support in the RedisStore first.

/Users/Till/Development/Repositories/framework/tests/Integration/Cache/RedisStoreTest.php:88

Illuminate\Tests\Integration\Cache\RedisStoreTest::testItCanAddWithTtl with data set "phpredis_prefix" ('phpredis_prefix')
Needs support in the RedisStore first.

/Users/Till/Development/Repositories/framework/tests/Integration/Cache/RedisStoreTest.php:88

Illuminate\Tests\Integration\Cache\RedisStoreTest::testItCanAddWithTtl with data set "phpredis_scan_noretry" ('phpredis_scan_noretry')
Needs support in the RedisStore first.

/Users/Till/Development/Repositories/framework/tests/Integration/Cache/RedisStoreTest.php:88

Illuminate\Tests\Integration\Cache\RedisStoreTest::testItCanAddWithTtl with data set "phpredis_scan_retry" ('phpredis_scan_retry')
Needs support in the RedisStore first.

/Users/Till/Development/Repositories/framework/tests/Integration/Cache/RedisStoreTest.php:88

Illuminate\Tests\Integration\Cache\RedisStoreTest::testItCanAddWithTtl with data set "phpredis_scan_prefix" ('phpredis_scan_prefix')
Needs support in the RedisStore first.

/Users/Till/Development/Repositories/framework/tests/Integration/Cache/RedisStoreTest.php:88

Illuminate\Tests\Integration\Cache\RedisStoreTest::testItCanAddWithTtl with data set "phpredis_scan_noprefix" ('phpredis_scan_noprefix')
Needs support in the RedisStore first.

/Users/Till/Development/Repositories/framework/tests/Integration/Cache/RedisStoreTest.php:88

Illuminate\Tests\Integration\Cache\RedisStoreTest::testItCanAddWithTtl with data set "phpredis_serializer_php" ('phpredis_serializer_php')
Needs support in the RedisStore first.

/Users/Till/Development/Repositories/framework/tests/Integration/Cache/RedisStoreTest.php:88

Illuminate\Tests\Integration\Cache\RedisStoreTest::testItCanAddWithTtl with data set "phpredis_serializer_igbinary" ('phpredis_serializer_igbinary')
Needs support in the RedisStore first.

/Users/Till/Development/Repositories/framework/tests/Integration/Cache/RedisStoreTest.php:88

Illuminate\Tests\Integration\Cache\RedisStoreTest::testItCanAddWithTtl with data set "phpredis_serializer_json" ('phpredis_serializer_json')
Needs support in the RedisStore first.

/Users/Till/Development/Repositories/framework/tests/Integration/Cache/RedisStoreTest.php:88

Illuminate\Tests\Integration\Cache\RedisStoreTest::testItCanAddWithTtl with data set "phpredis_compression_lzf" ('phpredis_compression_lzf')
Needs support in the RedisStore first.

/Users/Till/Development/Repositories/framework/tests/Integration/Cache/RedisStoreTest.php:88

Illuminate\Tests\Integration\Cache\RedisStoreTest::testItCanAddWithTtl with data set "phpredis_compression_zstd" ('phpredis_compression_zstd')
Needs support in the RedisStore first.

/Users/Till/Development/Repositories/framework/tests/Integration/Cache/RedisStoreTest.php:88

[TheLevti]

This is fine. I added those as a preparation for my next pull request. Currently the Cache component does not support any of the serializer/compressors. I want this test suite improvement to be merged first before I submit the next pull request that adds this functionality

[tillkruss]

Currently the Cache component does not support any of the serializer/compressors. I want this test suite improvement to be merged first before I submit the next pull request that adds this functionality

Ah nice, I like where you're going with this!

[driesvints]

@TheLevti if we can't fix the tests then for now it's probably best to don't go forward with this.

[TheLevti]

@TheLevti if we can't fix the tests then for now it's probably best to don't go forward with this.

Hey, I just did not have time in the last few weeks. I will check it out now, giving it another try. 💪🏻

[TheLevti]

@driesvints @tillkruss @taylorotwell could you please try again? I updated the test.sh to support any platform. It seems like it was written specifically for mac os only. It should now run on mac os, linux or windows WSL the same way.

In addition I noticed that sometimes windows tests failed (https://github.com/laravel/framework/runs/7163912435?check_suite_focus=true), because the active_url validator test was doing real DNS lookups, which must be avoided during test runs as sometimes this times out or fails. This is now fixed by mocking the dns record fetching.

The added changes are:

• Make sure host gateway is mapped to a generic host name that is available on mac os/linux/windows.
• Make sure to mount the file system with the same user as the host so that we do not have inconsistencies when files are written by tests.
• Mock actual DNS lookup calls (dns_get_record).

[taylorotwell]

Output is still all messed up for me. I don't really have time atm to take on such a big PR. Sorry 🫤

[TheLevti]

Output is still all messed up for me. I don't really have time atm to take on such a big PR. Sorry 🫤

Can someone please tell me what is wrong with the output?

The proposed changes are not related and ci/tests are passing perfectly fine for a long time now.

I suspect we still have a non os independent local test setup, which causes some trouble. I asked several of my colleagues to git clone and run the test script on multiple different machines and all finish without a problem now. Even though we found some more points that can be improved for the local test run.

[tillkruss]

Tests run fine for me on macOS 12.4 using iTerm2 3.4.16.

[driesvints]

@TheLevti I think the best course of action for now might be to send in smaller incremental PR's to see if we can solve this and spot the output bug in one of them. Like Taylor indicated, it will probably be impossible to review such a large PR.

[TheLevti]

@TheLevti I think the best course of action for now might be to send in smaller incremental PR's to see if we can solve this and spot the output bug in one of them. Like Taylor indicated, it will probably be impossible to review such a large PR.

I would like to avoid losing this changes as they have been already reviewed by others and the final benefit is worth the effort. But if Tylor has the last say, I will of course try with smaller chuncks if this is the only way.

It would be helpful to get the locally broken output reproduced though. If anyone could contribute with their output plus terminal they use where it is broken, this would be nice.

My suggestion here then would be:

1. pr to mock external dns check.
2. pr to improve local test runs.
3. pr (1 or more) to improve the redis test suite (it cant be a lot smaller as i need to update all test classes that use redis to make sure that all future redis changes are properly tested (using data providers)

Would that work or can we get this pull request fixed somehow together?

Anyone knows what machine + which terminal Tylor is using?

[driesvints]

@taylorotwell ^

[tillkruss]

Yeah, it's hard to make this PR smaller. Maybe someone who gets the wonky output can do a quick call for diagnostics?

[TheLevti]

Does it happen to you when you do a fresh git clone and run the tests on your machine @driesvints as for you the output was also broken before? I suspect some old not tracked files/folders with an incorrect user/acl cause some issues.

[driesvints]

I can confirm I no longer see the weird test output. I have no idea why that is... I ran ./bin/test/sh and then phpunit and it properly ran the tests for me.

@TheLevti we recently made a move to remove as many skipped output as possible. However, this PR re-adds a bunch of that, making it harder to dig into failed tests in the CI. Can you do this differently maybe?

[TheLevti]

I can confirm I no longer see the weird test output. I have no idea why that is... I ran ./bin/test/sh and then phpunit and it properly ran the tests for me.

@TheLevti we recently made a move to remove as many skipped output as possible. However, this PR re-adds a bunch of that, making it harder to dig into failed tests in the CI. Can you do this differently maybe?

I added those tests, because I wanted to add the necessary support in the follow up pull request where those tests would run successfully. Shall I leave them and wait for the next pull request or get them out? My follow up pull request will add proper support on the cache component. I just first wanted to clean up redis tests.

[driesvints]

It's best to filter out this before we merge. Otherwise it's going to be annoying to review other pr's or failing builds where test fail.

[TheLevti]

It's best to filter out this before we merge. Otherwise it's going to be annoying to review other pr's or failing builds where test fail.

Ok, sure. Will re-add them once the relevant code will be added. Thank you for the input 🥳

[TheLevti]

@driesvints Incomplete tests are gone. What are the next steps?

[driesvints]

@taylorotwell will review this next