Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Ensure freezeUuids always resets UUID creation after exception in callback #44018

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

lioneaglesolutions
Copy link
Contributor

@lioneaglesolutions lioneaglesolutions commented Sep 6, 2022

If merged, #44013 should be closed.

Please see #44013 for details on the problem this PR is solving.

This PR is a slightly different implementation of #44013 using the existing method as per the recommendation from @timacdonald #44013 (comment).

Instead of messy commits I thought 2 separate PRs would be cleaner.

Only one of them should be merged.


Recently I ran into an issue using the new createUuidsUsing method introduced in #42619.

The issue caused most of my test suite to fail due to one test failing.

This is because I use Str::uuid() to generate UUIDs for my models and I was getting a heap of duplicates. The underlying cause being that when my initial test failed, Str::createUuidsNormally() was never being called to essentially clean up from an exception/failed assertion that I was not expecting to be thrown.

Then when running the test in isolation, it works fine. I don't think failures in previous tests should cause all of your test suite to fail.

So as an example, both tests below would fail;

class UuidTest extends FeatureTestCase
{
    /** @test */
    public function it_sets_uuid()
    {
        Str::createUuidsUsing(fn () => Str::of('1234'));

        // It was actually a Storage::assertExists() failure in my case.
        throw new Exception('Test failed.');

        Str::createUuidsNormally();
    }

    /** @test */
    public function it_creates_uuids_normally_after_previous_failed_tests()
    {
        $this->assertFalse(Str::orderedUuid()->toString() === '1234'); 
        // FAIL as it's still using '1234' from above
    }
}

This then caused hundreds of tests to fail. With this new PR, when considering the following example, the first test will fail (as expected) however the second test will pass.

class UuidTest extends FeatureTestCase
{
    /** @test */
    public function it_sets_uuid()
    {
        Str::freezeUuids(function () {
            Str::createUuidsUsing(fn () => Str::of('1234'));

            throw new Exception('Test failed.');
        });
    }

    /** @test */
    public function it_creates_uuids_normally_after_previous_failed_tests()
    {
        $this->assertFalse(Str::orderedUuid()->toString() === '1234');
        // PASS as `Str::createUuidsNormally();` will always be called after the callback
        // in `freezeUuids`
    }
}

I believe similar logic would already apply to methods like Event::fakeFor() where failures within that closure would not affect additional tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants