laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.21k stars 10.9k forks source link

Testing notifications: toMail function is not reachable during testing #18910

Closed mabumusa1 closed 7 years ago

mabumusa1 commented 7 years ago

Description:

Raised exceptions within a notification class are not logged into laravel.log and not reported to PHPUnit, any raised exception inside the method toMail($notifiable) is not tracked during mocking

Steps To Reproduce:

  1. Create a simple route as follows, it just fire the notification
    
    Route::get('/bug', function(Request $request){
    $managers = \App\User::where('user_class', 'tester')->first();
    Notification::send($managers, new \App\Notifications\NotifyTesterWithTest());
    });

2. Create a test for this route.
public function testCustomerDashboard()
{
  Notification::fake();
  $response = $this->actingAs($this->tester)->get("bug");
  Notification::assertSentTo([$this->tester], NotifyTesterWithTest::class);
}

3. Stop the execution of `toMail` function or make a syntax error 
public function toMail($notifiable)
{
  dd('ss');
    return (new MailMessage)
                ->line('You have been selected for a test')
                ->action('Please visit your dashboard to take the test', route('tester_dashboard'))
                ->line('Thanks for working with Users Proof');
}
4- Run the test.

phpunit --group=BugController PHPUnit 5.7.16 by Sebastian Bergmann and contributors.

. 1 / 1 (100%)

Time: 409 ms, Memory: 10.00MB

OK (1 test, 1 assertion)



The test passes despite there is a die inside the code. I added `dd` to the `constructor` and the `via` function it works well. but I can not trace it inside `toMail` function. I also could not find the errors (when I made a syntax error) inside `laravel.log`

Full code 
NotifyTesterWithTest.php https://paste.laravel.io/0MV7z 
web.php https://paste.laravel.io/p4OYO
Test https://paste.laravel.io/Ro1QK
mabumusa1 commented 7 years ago

Yes, it is configured with mail only

On Mon, Apr 24, 2017, 12:25 AM Mohamed Benhida notifications@github.com wrote:

did you add mail to notifiable function in notification controlle public function via($notifiable) { return ['broadcast','database','mail']; }

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/laravel/framework/issues/18910#issuecomment-296489770, or mute the thread https://github.com/notifications/unsubscribe-auth/AMCuyk5-YU0NL-BoFfubd4prX1_mJaYlks5ry8GsgaJpZM4NFUwH .

themsaid commented 7 years ago

Yes because we don't test the actual sending, we only check that a notification is sent. That's why the method is never called.

mabumusa1 commented 7 years ago

How can you test the actual sending? I tried to fake a mail to verify the message but I failed to do so.

We had an exception in the mail template of the notification and testing did not catch that

We had to test the notification through a web route to detect the issue.

Any suggestions to test this better?

On Mon, Apr 24, 2017, 4:38 PM Mohamed Said notifications@github.com wrote:

Yes because we don't test the actual sending, we only check that a notification is sent. That's why the method is never called.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/laravel/framework/issues/18910#issuecomment-296671030, or mute the thread https://github.com/notifications/unsubscribe-auth/AMCuyinnVPe5HjkY0dgInszOr339ylbxks5rzKXogaJpZM4NFUwH .

stemis commented 7 years ago

@mabumusa1

How can you test the actual sending? I tried to fake a mail to verify the message but I failed to do so.

Did you try to not fake the notification but fake the mail only?

mabumusa1 commented 7 years ago

@stemis I did this test before, but it did not work.

I think we need to figure out how to fire the function toMail first to detect if the message is being sent, What are your thoughts?

I still can not reach the function toMail on any of the trials I mentioned above. I did not test other via like database so I can not judge on all the cases. Maybe we need to figure out when the function toMail is being called

      Mail::fake();
          Mail::assertSent(MailMessage::class, function ($mail) use ($tester) {
            dd($tester);
        });

This is the result of the test


PHPUnit 5.7.19 by Sebastian Bergmann and contributors.

F                                                                   1 / 1 (100%)

Time: 2.15 seconds, Memory: 12.00MB

There was 1 failure:

1) Tests\Feature\BatchesControllerTest::testpublishBatch
The expected [Illuminate\Notifications\Messages\MailMessage] mailable was not sent.
Failed asserting that false is true.

/home/user/usersproof/htdocs/vendor/laravel/framework/src/Illuminate/Support/Testing/Fakes/MailFake.php:29
/home/user/usersproof/htdocs/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php:221
/home/user/usersproof/htdocs/tests/Feature/Controllers/BatchesControllerTest.php:362
/home/user/usersproof/htdocs/vendor/laravel/framework/src/Illuminate/Support/Testing/Fakes/EventFake.php:65
/home/user/usersproof/htdocs/vendor/laravel/framework/src/Illuminate/Support/Arr.php:517
/home/user/usersproof/htdocs/vendor/laravel/framework/src/Illuminate/Support/Collection.php:321
/home/user/usersproof/htdocs/vendor/laravel/framework/src/Illuminate/Support/Testing/Fakes/EventFake.php:66
/home/user/usersproof/htdocs/vendor/laravel/framework/src/Illuminate/Support/Testing/Fakes/EventFake.php:27
/home/user/usersproof/htdocs/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php:221
/home/user/usersproof/htdocs/tests/Feature/Controllers/BatchesControllerTest.php:364
jhoff commented 7 years ago

Just wanted to mention that it is possible to test the toMail ( or any other method for that matter on a notification ) but you just have to trigger it manually:

Notification::assertSentTo(
    $user,
    EmailVerification::class,
    function ($notification) use ($user) {
        $mailData = $notification->toMail($user)->toArray();

        $this->assertContains('Some expected line in the intro', $mailData['introLines']);
        $this->assertEquals('Click Me!', $mailData['actionText']);
        $this->assertEquals(route('email.verify', $notification->token), $mailData['actionUrl']);
        $this->assertContains('Some expected line in the outro', $mailData['outroLines'][0]);
        // ...
        return true;
    }
);
mabumusa1 commented 7 years ago

Thanks @jhoff

This is a very smart approach, I think it is valuable to be inside the documentation as well

MattStrauss commented 7 years ago

This would be useful as in some case the mail is optionally sent within the Notification per the documentation:

public function via($notifiable) { return $notifiable->prefers_sms ? ['nexmo'] : ['mail', 'database']; }

kbond commented 5 years ago

I wanted my test to inspect the actual \Swift_Message sent via a notification.

To do this, I get the sent messages via the array transport (the default for tests):

app('mailer')->getSwiftMailer()->getTransport()->messages()

And wrote some custom assertions around this.

Ensure your tests do not have Mail::fake() or Notification::fake() enabled.

I couldn't get this to work with queued notifications but I didn't look to deep into why. Maybe this can help someone else.