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

[8.x] Added assertRedirectToSignedRoute() method for testing responses #38349

Merged
merged 7 commits into from
Aug 13, 2021

Conversation

ash-jc-allen
Copy link
Contributor

Hey! This PR adds a new method called assertRedirectToSignedRoute() that works in an almost identical way to the assertRedirect() route. I might be approaching this from the wrong angle, but I've not found a clean way of testing signed URL redirects so far, so I thought this could be pretty useful.

Here's an example of a controller method that we could write the test for:

class RandomController extends Controller
{
    public function __invoke()
    {
        // Do something...
        
        return redirect()->signedRoute('example.route', ['param' => 'hello']);
    }
}

For example's sake, I'll just say that the route name for the method above is laravel.index... Now in the test we could use:

public function test_user_is_redirected_to_signed_route()
{
    // We could do this if we just want to check that we were redirected to a signed URL
    $this->get(route('laravel.index'))
        ->assertRedirectToSignedRoute();

    // We could do this if we just want to check the URL was correct
    $this->get(route('laravel.index'))
        ->assertRedirectToSignedRoute(route('example.route', ['param' => 'hello']));
}

I've been trying this with quite a few different types of signed URLs (non-expiring and temporary) and it seems to cover all of the use cases I've tried. But it's possible that I might have missed some edge cases maybe?

I've not written any test for this, but if it's something you think might be worth pulling in, I'd be happy to write some tests for it 😄

@taylorotwell
Copy link
Member

It's a little unintuitive to me that I need to pass a URI to this method. I would expect to need to pass a route name and an array of parameters (optional). Basically the same signature as the route helper. If you could make that change and add test that would be good. Marking this as draft for now. Please mark as ready for review when done.

@taylorotwell taylorotwell marked this pull request as draft August 12, 2021 12:54
@ash-jc-allen
Copy link
Contributor Author

@taylorotwell Actually yeah, I fully agree with that, I think it'd definitely make more sense. I'll make the updates now, cheers :)

@ash-jc-allen
Copy link
Contributor Author

I've updated the method now based on your suggestions so you can use it like this:

// We could do this if we just want to check that we were redirected to a signed URL
    $this->get(route('laravel.index'))
        ->assertRedirectToSignedRoute();

    // We could do this if we just want to check the URL was correct
    $this->get(route('laravel.index'))
        ->assertRedirectToSignedRoute('example.route', ['param' => 'hello']);

I've also added some tests for it too. I'm not quite sure if I've put the tests in the right place or written them properly, so I'm happy to make any changes if needed

@ash-jc-allen ash-jc-allen marked this pull request as ready for review August 12, 2021 14:16
@taylorotwell
Copy link
Member

Are the tests passing for you locally? Looks like they are failing on here.

@ash-jc-allen
Copy link
Contributor Author

Yeah I've just had a quick check locally and if I run the one test file on it's own, they all pass... But if I run the entire test suite, it keeps failing on \Illuminate\Tests\Testing\Concerns\TestDatabasesTest::testSwitchToDatabaseWithoutUrl.

I must have messed something up, so I'll take a look at it now 🤔

@ash-jc-allen
Copy link
Contributor Author

Looks like my tests weren't tearing down properly at the end, so I've just made a quick update. They seem to be passing now 🙂

@taylorotwell taylorotwell merged commit f3470df into laravel:8.x Aug 13, 2021
@ash-jc-allen ash-jc-allen deleted the assert-signed-route-redirect branch August 13, 2021 13:24
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