Как я пытался улучшить Laravel, а сделал только хуже

Вступление

Laravel – классный PHP-фреймворк, мы им постоянно пользуемся в компании. Но как известно, ничто в мире не идеально, можно всегда предложить улучшения.

Несколько недель назад я попытался сделать одно маленькие улучшение по части тестов в Laravel, открыл два пулл-реквеста (#1 и #2). Оба пулл-реквеста были отклонены автором фреймворка Тейлором, но в итоге он сам в этот же день опубликовал собственную реализацию того же функционала, о чём даже в твиттере похвалился. И, о боги, реализацию ужасную!

Контекст

Мы предпочитаем интеграционное тестирование у себя, так как оно позволяет достигнуть хорошего баланса между стоимостью автоматического тестирования и уверенностью в своих деплоях. Зачастую у себя в интеграционных тестах мы что-то делаем от имени пользователя, а потом хотим убедиться, что пользователю в итоге пришло какое-то письмо. Для этого в Laravel реализован стандартный способ:

<?php
public function test_orders_can_be_shipped()
{
    Mail::fake();

    Mail::assertSent(OrderShipped::class);
}

Это, конечно, хорошо, но что если внутри этого почтового класса есть какая-то логика, которая влияет на содержание письма? К примеру, может придти "Заказ отменен - оплата не прошла", а может придти "Заказ отменен - товара нет на складе".

В Laravel в тесте можно добавить еще коллбек, чтобы делать дополнительные проверки:

<?php
Mail::assertSent(OrderShipped::class, function ($mail) use ($user) {
    return $mail->hasTo($user->email) &&
           $mail->somePublicProperty == 'someValue';
});

Но всё, что тут можно проверить - это публичные переменные на нашем почтовом классе и поля имейла вроде To/Cc/Bcc (непонятно, что тут эти методы делают, если честно). Класс Mailable в Laravel реализован так, что никак нельзя получить доступ к уже срендеренным письмам. Когда мы вызываем метод render() на Mailable, происходит следующее:

<?php
public function render()
{
    return $this->withLocale($this->locale, function () {
        Container::getInstance()->call([$this, 'build']);
        return Container::getInstance()->make('mailer')->render(
            $this->buildView(), $this->buildViewData()
        );
    });
}

Метод находит текущий почтовый сервис в контейнере и на нем вызываем метод render(), то есть выполняет роль посредника. В качестве параметров передает ему частично срендеренные шаблоны, используя защищенные методы buildView() и buildViewData() - из тестов к ним обращаться нельзя.

Итак, оставались следующие опции для тестирования содержимого писем:

• Расширить класс Mailable, тогда мы получим доступ к защищенным методам. Всем почтовым классам отныне придется расширять этот новый класс. Всем разработчикам в мире придется это повторять снова и снова.

• Добавить на класс Mailable трейт Macroable, чтобы его можно было динамически расширять. Тут проблема в том, что Mailable уже имеет магический метод для всего, что начинается с "with", а трейт Macroable не поддерживает маски. Простого решения нет.

• Создать какой-то класс-посредник, proxy, который будет брать объект с Mailable, будет ему передавать все запросы, давать доступ ко всем его публичным переменным, но также добавит сверху несколько методов, которые нужны только в тестах - вроде seeInHtml(), seeInText(). Эту опцию я и выбрал.

Предложенное решение

Моё предложенное решение (второй пулл-реквест) использовало пакет Mockery, чтобы динамически расширять классы Mailable во время тестов, добавляя им новые методы:

<?php
public function email_confirmation_is_correct()
{
    Mail::fake();

    event(new TestEvent());
    config(['app.name' => 'Test App']);

    Mail::assertSent(TestMail::class, function (TestMail $mail) {
        return $mail->hasTo('test@test.com') 
          && $mail->seeInHtml('shipped')
          && $mail->dontSeeInHtml('failed') 
          && $mail->seeInText('Test App');
        });
    }

Выглядит просто, API очень похож на тестирование веб-запросов с Laravel, и все что мы изменили в коде - это только классы, которые имеют отношение к тестированию (немного изменили MailFake, добавили класс TestMailable). Ни одного production-класса мы не трогали.

Уже казалось, что приличное решение найдено, но второй пулл-реквест все-таки был также отклонен и в этот же день Тейлор делает следующий твит:

И тут я, конечно, обрадовался - быть может, это не мой код, но главное ведь, что фича, которую я ждал увидела свет - ура! И API практически такой же, разве что с префиксом "assert".

Мне стало интересно посмотреть в код – как же сам Тейлор реализовал это, учитывая, что ему не понравилось моё решение.

Ужас

Качество кода – вещь, понятно, субъективная, и порой можно часами спорить безрезультатно. Но я всё же попробую объяснить почему я считаю, что код Тейлора – тихий ужас.

Тейлор взял и залез в production-класс Mailable, класс, который я не хотел трогать. Ну да ладно, его фреймворк, ему виднее.

Он добавил подобные методы, которые будут вызываться в тестах, прямо в Mailable:

<?php
/**
 * @param  string  $string
 * @return void
 */
public function assertSeeInText($string)
{
    [$html, $text] = $this->renderForAssertions();

    PHPUnit::assertTrue(
        Str::contains($text, $string),
        "Did not see expected text [{$string}] within text email body."
    );
}

Что по моему мнению уже плохо:

• Для всех остальных классов и фич в Laravel, тестовая часть достаточно хорошо отделена от production-части, находится в своих пространствах имен и классах. Может это мелочь, но как-то приятно когда все отсортировано. В идеале, я бы хотел видеть эти новые методы где-то или в классе MailFake, или в каком-то новом классе для тестовых Mailable

• Класс Mailable теперь импортирует класс PHPUnit, а тот является зависимостью development-only, судя по файлу composer.json. Таким образом, на тысячах production-серверов в мире будет строка с импортом чисто тестовой зависимости

Но хуже всего вот это:

[$html, $text] = $this->renderForAssertions();

Получается, что мы тестируем не тот метод, который рендерит наши настоящие письма. Вместо этого, мы создали совершенно новый, специальный метод, который рендерит письма лишь для наших тестов.

Наша цель была протестировать боевой код – методы render() и send(). Мы же протестировали только что созданный новый метод, который никем не будет использоваться на практике.

Давайте-ка взглянем на этот метод:

<?php
protected function renderForAssertions()
{
    if ($this->assertionableRenderStrings) {
        return $this->assertionableRenderStrings;
    }

    return $this->assertionableRenderStrings = $this->withLocale($this->locale, function () {
        Container::getInstance()->call([$this, 'build']);

        $html = Container::getInstance()->make('mailer')->render(
            $view = $this->buildView(), $this->buildViewData()
        );

        $text = $view['text'] ?? '';

        if (! empty($text) && ! $text instanceof Htmlable) {
            $text = Container::getInstance()->make('mailer')->render(
                $view['text'], $this->buildViewData()
            );
        }

        return [(string) $html, (string) $text];
    });
}

Ой, не метод, а монстр!

Его не только очень сложно читать, он ещё повторяет очень много строк из метода render(), так что теперь есть шанс что когда-нибудь методы разойдутся. И каждый раз редактируя render(), теперь надо не забыть поправить и renderForAssertions().

Я считаю, что это принципиально неправильно – тестировать специально созданные для тестов методы, вместо настоящего кода. Это какая-то бессмыслицы.

Я считаю, что с этим коммитом мой любимый фреймворк стал немного хуже :(

Автор: Denis Mysenko

Источник