Problem/Motivation

As part of #2847428: [Meta] Modernize dblog module and #2807237: PHPUnit initiative the test that ensures dblog_cron works fine could be moved from the WebTestBase into a Kernel test.

Proposed resolution

Extract the DbLogTest::verifyCron into a kernel test.

Remaining tasks

Write a patch.

User interface changes

None

API changes

New Kernel test class.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dagmar created an issue. See original summary.

dagmar’s picture

Issue summary: View changes
dagmar’s picture

Issue tags: +phpunit initiative

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ApacheEx’s picture

Status: Active » Needs review
FileSize
8.11 KB

Here is a patch.

I'd like to highlight:
1) was changed $cron_count = 1 to $cron_count == 1 which is more correct.
2) was renamed verifyCron to testDbLogCron.

dawehner’s picture

I'm curious whether we should improve the code while porting it to a kernel test or not.

  1. +++ b/core/modules/dblog/tests/src/Kernel/DbLogTest.php
    @@ -0,0 +1,129 @@
    +    $this->assertTrue($count > $row_limit, format_string('Dblog row count of @count exceeds row limit of @limit', ['@count' => $count, '@limit' => $row_limit]));
    

    You could use assertGreaterThan here

  2. +++ b/core/modules/dblog/tests/src/Kernel/DbLogTest.php
    @@ -0,0 +1,129 @@
    +    $this->assertTrue($cron_detailed_count == $module_count + 2, format_string('Cron added @count of @expected new log entries', ['@count' => $cron_detailed_count, '@expected' => $module_count + 2]));
    

    You could use assertEquals here

  3. +++ b/core/modules/dblog/tests/src/Kernel/DbLogTest.php
    @@ -0,0 +1,129 @@
    +    $this->assertTrue($cron_count == 1, format_string('Cron added @count of @expected new log entries', ['@count' => $cron_count, '@expected' => 1]));
    

    You could use assertEquals here as well

ApacheEx’s picture

FileSize
8.12 KB
1.83 KB

thanks for your review. I'm always want to improve smth here :). Then I read this:

Otherwise try to change as little as possible in the converted web test. Do not get tempted to "improve" or "tidy up" the tests - the conversion should be easy to review with only the minimum necessary changes. Further cleanup can be done in a follow-up issue.

and try to convert with minimal changes. Anyways, Here is improved patch.

dagmar’s picture

Status: Needs review » Postponed

Thanks for working on this! But I think we should fix #2848914: Move DbLogTest::generateLogEntries() into a Trait to avoid code duplication on the code duplication on generate log entries.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dagmar’s picture

Status: Postponed » Needs work

#2848914: Move DbLogTest::generateLogEntries() into a Trait was committed. This issue is unblocked now.

longwave’s picture

This needs #2985701: Followup: Move DbLogTest::generateLogEntries() into a Trait first as $this->adminUser is not set during the kernel test.

ApacheEx’s picture

Status: Needs work » Needs review
FileSize
5.99 KB
longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 41b994cc88 to 8.7.x and 9130e65fbe to 8.6.x. Thanks!

Backported to 8.6.x since this is a test only change.

  • alexpott committed 41b994c on 8.7.x
    Issue #2848529 by ApacheEx, dagmar, dawehner: Move DbLogTest::verifyCron...

  • alexpott committed 9130e65 on 8.6.x
    Issue #2848529 by ApacheEx, dagmar, dawehner: Move DbLogTest::verifyCron...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.