[9.x] Improve redis test suite

[TheLevti]

This is a follow up of #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?

[tillkruss]

This looks good to me. Thank you split-diff-view.

[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]

@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

[taylorotwell]

I still get a bunch of errors locally - I have no idea. I again don't really have a ton of time to dig into this and it's a huge change to our test setup for Redis that I won't know anything about. Is there a particular bug you want to fix in Redis that exists in Laravel right now? If so, can we just fix that and add a test for it?

[TheLevti]

I still get a bunch of errors locally - I have no idea. I again don't really have a ton of time to dig into this and it's a huge change to our test setup for Redis that I won't know anything about. Is there a particular bug you want to fix in Redis that exists in Laravel right now? If so, can we just fix that and add a test for it?

Please try to run the test suite with a fresh git clone or workspace clean up (files being generated with www-data or root user must be gone).

No, this is not a bug fix, but a cleanup for the current redis tests, because they are not well structured and each component that uses redis has a non consistent test flow.

This pull request would first make sure that everything that uses redis, would use the same data providers. Its all about that.

I wanted to do this to make sure that the planned follow up pull requests will make sure that everything still works.

I will then just provide the follow up pull request with just the necessary tests.