Problem/Motivation

This is follow-up issue from #3496117: Remove unnecessary usage of dblog module at tests.

Some tests use dblog module to check log messages, this requires database connection.
In order to simplify this and improve test speed, we can use some alternatives.

We already has dependency on

  • \Symfony\Component\ErrorHandler\BufferingLogger
  • \ColinODell\PsrTestLogger\TestLogger

BufferingLogger fails if on tests end there are some unhadled errors. It can be used in case you need to be sure there is no error messages on test finish.

TestLogger allows to have log messages on test finish, and much more flexible. The class is final, it's not possible to extend.

But both of these classes collect all context, and if you just need to check log message and log message variables, you need to filter log context.

Steps to reproduce

Proposed resolution

To improve DX, and allow to easily check logs during tests:
1) Create proxy class for \ColinODell\PsrTestLogger\TestLogger and filter context to contain only message variables.
2) Replace existing usage of dblog module at Kernel test with new proxy class for

  • Drupal\Tests\comment\Kernel\CommentIntegrationTest
  • Drupal\Tests\field_ui\Kernel\EntityDisplayTest
  • Drupal\Tests\image\Kernel\ImageItemTest
  • Drupal\Tests\jsonapi\Kernel\ResourceType\RelatedResourceTypesTest
  • Drupal\KernelTests\Core\Action\EmailActionTest

3) As follow-up we also can convert existing usage of \Symfony\Component\ErrorHandler\BufferingLogger,\ColinODell\PsrTestLogger\TestLogger and other approaches to new proxy class. So all code base use similar approach.

Remaining tasks

These functional tests maybe we can also make replacement, but this need to be discussed.

  • Drupal\Tests\big_pipe\Functional\BigPipeTest
  • Drupal\Tests\media\FunctionalJavascript\MediaSourceOEmbedVideoTest
  • Drupal\Tests\node\FunctionalNodeCreationTest
  • Drupal\Tests\system\Functional\Module\ModuleTestBase

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3496184

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

nikolay shapovalov created an issue. See original summary.

nikolay shapovalov’s picture

Issue summary: View changes
nikolay shapovalov’s picture

These tests:

  • Drupal\Tests\comment\Kernel\CommentIntegrationTest
  • Drupal\Tests\field_ui\Kernel\EntityDisplayTest
  • Drupal\Tests\image\Kernel\ImageItemTest
  • Drupal\Tests\jsonapi\Kernel\ResourceType\RelatedResourceTypesTest
  • Drupal\KernelTests\Core\Action\EmailActionTest

have same goal. Find single message and check variables. Maybe we can create trait for this, to be able to register logger and check message.

Example:

    $logged = Database::getConnection()->select('watchdog')
      ->fields('watchdog', ['variables'])
      ->condition('type', 'jsonapi')
      ->condition('message', 'The "@name" at "@target_entity_type_id:@target_bundle" references the "@entity_type_id:@bundle" entity type that does not exist.')
      ->execute()
      ->fetchField();
    $this->assertEquals(serialize($arguments), $logged);

nikolay shapovalov’s picture

I create trait that register \Symfony\Component\ErrorHandler\BufferingLogger as logger service.
And have new assert method.
I found that using BufferingLogger have some cons:

While working CommentIntegrationTest.php, there was one error log that was not checked by tests, that caused error fail on test finish. Because of BufferingLogger::__desctruct() showing all errors on test finish.
Also BufferingLogger::__desctruct() method doesn't support drupal token replacement.
I believe this is not behavior I want.
If tests doesn't care about log, we also need to ignore that.

Also I don't like implementing register() method at trait. Maybe we can directly inject logger to KernelTest.

nikolay shapovalov’s picture

I decide that it would be great to have TestLogger to use at tests as dblog alternative.
And found that we already using \ColinODell\PsrTestLogger\TestLogger at some tests.
Another great example is \Drupal\Tests\feeds\Kernel\TestLogger and Drupal\Tests\feeds\Kernel\FeedsKernelTestBase.

Maybe we can use \ColinODell\PsrTestLogger\TestLogger::hasRecordThatPasses to find message with argument. So we don't need to reinvent the wheel.

nicxvan’s picture

That would be interesting! Just in case you didn't know, adding a dependency to core has a lot of gates:

https://www.drupal.org/about/core/policies/core-dependency-policies/depe...

Might be worth asking in the core development channel for advice before going too far down this path.

Here is the project: https://github.com/colinodell/psr-testlogger

nikolay shapovalov’s picture

@nicxvan thanks for feedback.
But as I can see colinodell/psr-testlogger is already a part of Core dependency.

I will ask for advice anyway, thanks. Just want to make some research before, to have better understanding of the topic.

nicxvan’s picture

Oh sorry, I thought were researching a new dependency, I misunderstood!

nikolay shapovalov’s picture

Status: Active » Needs review

I create helper class TestLogger. Attaching it directly in the test method looks very easy, and hasn't a lot of boilerplate.
I was thinking to use colinodell/psr-testlogger, but there are a lot of data in the log->context. In order to filter it, we can use proxy and facade approach, but I want to keep it simple. Creating custom class looks like good alternative.
Also I added rendered message to log, and TestLogger can search rendered message, this is not used in MR, but this should be useful for someone who is searching simple rendered message.

Set to NR to get some feedback.

nikolay shapovalov’s picture

I created a Proxy for \ColinODell\PsrTestLogger\TestLogger to filter context of log records.
And result looks good. All \ColinODell\PsrTestLogger\TestLogger methods can be used.
Probably this is better than creating custom TestLogger from scratch.

nikolay shapovalov changed the visibility of the branch 3496184-logger-test-class to hidden.

nikolay shapovalov changed the visibility of the branch 3496184-convert-smart-default-settings to hidden.

nikolay shapovalov’s picture

Issue summary: View changes
nikolay shapovalov’s picture

Issue summary: View changes
nikolay shapovalov’s picture

Status: Needs review » Closed (won't fix)