nunomaduro / laravel-mojito

🍹 A lightweight package for testing Laravel views in isolation
http://nunomaduro.com/
MIT License
373 stars 16 forks source link

Adds a callable generator for Mail and Notification assertions #14

Open peterfox opened 4 years ago

peterfox commented 4 years ago

Implements the feature described in issue #13. Adds a static make command that generates a callable that is compatible with both Mail and Notification assertions in Laravel that then calls a provided callable with a ViewAssertion as the first parameter making it possible to test the rendered email content.

SimoTod commented 4 years ago

Hi @peterfox thanks for your contribution and sorry for the late response, I missed those notifications.

I'm going to throw a spanner in the job, sorry. :)

I'm thinking that InteractsWithViews could have assertMail and assertNotification returning a ViewAssertion instance. Your initial example would then become something like

        Mail::fake();

        Mail::to('test@test.com')->send(new ExampleMailable());

        Mail::assertSent(ExampleMailable::class, function ($mail) {
            $this->assertMail($mail)->contains('Laravel');
            return true;
        });

which I find a bit simpler and easier to read.

What do you think? Personally, I find ViewAssertion::make(function (ViewAssertion $assertion) {a bit verbose (we need to repeat ViewAssertion twice) and, despite being compatible with the documentation, is still a bit different from the Laravel example. Also, being a static function, it means that we can't inject other variables with use and we can only test the view while the other syntax would allow us to test other properties as well.

For example, we can have

        Mail::assertSent(ExampleMailable::class, function ($mail) use ($user) {
            $this->assertMail($mail)->contains('Laravel');
            return $mail->hasTo($user->email)
        });
peterfox commented 4 years ago

thanks for your contribution and sorry for the late response, I missed those notifications.

No worries, happens to me all the time on my projects.

which I find a bit simpler and easier to read.

Yeah I agree this might be the better option in the end especially as I know L7 has changed the assert methods a little so this feature might break now. Only problem is I'm not sure if this will work as well for Notifications I guess I could do something like:

$this->assertNotification($notification, $notifiable)->assertContains('Laravel');

What's your thoughts on traits? Should I add these to the interactsWithViews trait or apply it to new ones for Mail and Notifications?

SimoTod commented 4 years ago

I think they can be added to the same trait for simplicity since they are still checking stuff on generated views.

We can call them assertMailView and assertNotificationView if we want to highlight that they are still view related.

Let's see what @nunomaduro thinks.

peterfox commented 4 years ago

@SimoTod sure, will hold off further changes until @nunomaduro takes a look

nunomaduro commented 4 years ago

@peterfox @SimoTod

        Mail::assertSent(ExampleMailable::class, function ($mail) use ($user) {
            $this->assertView($mail)->contains('Laravel'); // $mail implements the viewable contract right?
        });
peterfox commented 4 years ago

@nunomaduro As far as I'm aware no. Mailables are a bit complicated because they can have a view or have a markdown template. Same with notifications but also in a more complicated way because they're typically a MailMessage but can also provide a Mailable object.

We could do like this:

Mail::assertSent(ExampleMailable::class, function ($mail) use ($user) {
      $this->assertView($mail)->contains('Laravel');
});

Notification::assertSentTo($user, ExampleNotification::class, function ($notification, $notifiable) use ($user) {
      $this->assertView($notification->toMail($notifiable))->contains('Laravel');
});

Using instanceOf in the assertView method to detect if it's a View, MailMessage, or Mailable in the assertView call and then provide the ViewAssertion instance from that.

SimoTod commented 4 years ago

My 2 pence Mailable objects implement the Renderable contract but we would have to remove the type-hinting in InteractsWithViews::assertView so we can accept either a string or a Mailable. Then we would have to add a bunch of instanceof checks to execute slightly different code.

A separate method would typehint Mailable and it would know exactly what to do (same for Notification).

Something like (probably not complete, i did not test it)

    /**
     * Create a new view test case.
     */
    protected function assertView(string $view, array $data = [], array $mergeData = []): ViewAssertion
    {
        return new ViewAssertion(view($view, $data, $mergeData)->render());
    }

    /**
     * Create a new view test case from a Mailable.
     */
    protected function assertMailView(Mailable $mailable): ViewAssertion
    {
        return new $mailable->render();
    }

    /**
     * Create a new view test case from a Notification.
     */
    protected function assertNotificationView(Notification $notification): ViewAssertion
    {
        if (! method_exists($notification, 'toMail')) {
            throw WhateverException('not testable');
        }
        return new $notification->toMail(null)->render();
    }

I'm not fussy about that but, as long as it's documented, different methods for this cases shouldn't cause too much confusion and the underlying code would look a bit nicer.

peterfox commented 4 years ago

@SimoTod The notification signature would have to be:

/**
     * Create a new view test case from a Notification.
     */
    protected function assertNotificationView(Notification $notification, $notifiable): ViewAssertion
    {

    }

Or we'd have to do:

/**
     * Create a new view test case from a Notification.
     */
    protected function assertNotificationView($message): ViewAssertion
    {
        if ($message instanceOf Mailable) {
            // return ViewAssertion
        } else if ($message instanceOf MailMessage) {
            // return ViewAssertion
        }

        // throw Invalid Arg
    }

I generally lead to your solution over @nunomaduro if only because it doesn't risk breaking compatibility with what's already there and I think most people who use it will understand they need to use the extra methods for notifications and mailables.

SimoTod commented 4 years ago

It makes sense to me. Both approaches are doable and look nice. If we go with the single method, I would just support strings (normal views) and Renderable objects, leaving to users the task of calling toMail on the notification object in their tests (we can just provide an example in the documentation). That would keep the code clean enough since the big chunk of complexity is related to the Notifications logic.

peterfox commented 4 years ago

@nunomaduro @SimoTod you guys in consensus then that it should be done as individual methods inside the trait?

SimoTod commented 4 years ago

@peterfox We didn't discuss it since Nuno has been busy with pest lately but I think he preferred the single helper to keep the public API simple. As I mentioned last week, I think we could accept Renderables and strings and leave the notification logic to the final user. What do you think? Can you see any issues with this approach?

peterfox commented 4 years ago

@SimoTod I think that works fine for me. Shouldn't be too difficult, might end up being a lot of logic in one method, will have to consider how to break up some of that logic