Problem/Motivation

As part of #2847428: [Meta] Modernize dblog module the plan is to use Kernel Tests to test the dblog module without require an UI.

The candidates for a Kernel Test are:

Which requires generate log entries.

It would be nice to abstract the DbLogTest::generateLogEntries() method into a trait to reuse it in both Kernel and Web based tests.

Proposed resolution

  • Create a new Trait to generate log entries.
  • Use this trait in Drupal\dblog\Tests. and in Kernel Tests

Remaining tasks

User interface changes

None

API changes

A new Trait.

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dagmar created an issue. See original summary.

dagmar’s picture

Status: Active » Needs review
FileSize
11.04 KB
dagmar’s picture

Status: Needs review » Needs work
pk188’s picture

Status: Needs work » Needs review
FileSize
8.11 KB

Re rolled. Check once.

Status: Needs review » Needs work

The last submitted patch, 4: 2848914-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

pk188’s picture

Status: Needs work » Needs review
FileSize
8.12 KB
dagmar’s picture

Status: Needs review » Needs work
+++ b/core/modules/dblog/tests/src/Functional/FakeLogEntries.php
@@ -0,0 +1,62 @@
+    // This long URL makes it just a little bit harder to pass the link part of
+    // the test with a mix of English words and a repeating series of random
+    // percent-encoded Chinese characters.
+    $link = urldecode('/content/xo%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A%E9%85%B1%E5%87%89%E6%8B%8C%E7%B4%A0%E9%B8%A1%E7%85%A7%E7%83%A7%E9%B8%A1%E9%BB%84%E7%8E%AB%E7%91%B0-%E7%A7%91%E5%B7%9E%E7%9A%84%E5%B0%8F%E4%B9%9D%E5%AF%A8%E6%B2%9F%E7%BB%9D%E7%BE%8E%E9%AB%98%E5%B1%B1%E6%B9%96%E6%B3%8A-lake-isabelle');
+

Thanks @pk188. If you take a look to #2882525: Move suspicious string in DblogTest into a KernelTest you will realize that this string was removed from the test precisely to make this easier to review.

pk188’s picture

Status: Needs work » Needs review
FileSize
5.14 KB

Fixed #7.

dagmar’s picture

Status: Needs review » Needs work

Thanks @pk188. Here are some comments:

  1. +++ b/core/modules/dblog/tests/src/Functional/FakeLogEntries.php
    @@ -0,0 +1,57 @@
    +      'timestamp'   => REQUEST_TIME,
    

    In Drupal 8.4.x we have a replacement for REQUEST_TIME. We can use it here.

  2. +++ b/core/modules/dblog/tests/src/Functional/FakeLogEntries.php
    @@ -0,0 +1,57 @@
    +    $logger = $this->container->get('logger.dblog');
    

    This assumes the ->container object will be present. This can make harder to re-use the trait. I think would be better to use Drupal::service instead.

  3. +++ b/core/modules/dblog/tests/src/Functional/FakeLogEntries.php
    @@ -0,0 +1,57 @@
    +}
    \ No newline at end of file
    

    Missing newline.

pk188’s picture

Status: Needs work » Needs review
FileSize
5.16 KB

Thanks! for reviewing.
Updated according to #9.

Status: Needs review » Needs work

The last submitted patch, 10: 2848914-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

pk188’s picture

Status: Needs work » Needs review
FileSize
5.15 KB

Updated last patch. I think this should work.

dagmar’s picture

Status: Needs review » Needs work

Thanks @pk188

+++ b/core/modules/dblog/tests/src/Functional/FakeLogEntries.php
@@ -0,0 +1,57 @@
+      'timestamp'   => (int) \Drupal::request()->server->get('REQUEST_TIME'),

Please take a look to the Change record

pk188’s picture

Status: Needs work » Needs review
FileSize
5.13 KB

I updated the patch.

dawehner’s picture

-1 for changing anything in there. IMHO just moving the code is the right scope for this particular issue.

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.

dagmar’s picture

Status: Needs review » Needs work

Based on @dawehner comments, let's back to #9 comments, without the part of TIME_REQUEST.

ApacheEx’s picture

dagmar’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/dblog/tests/src/Functional/FakeLogEntries.php
    @@ -0,0 +1,63 @@
    +   *   Number of watchdog entries to generate.
    

    We can replace watchdog with just log

  2. +++ b/core/modules/dblog/tests/src/Functional/FakeLogEntries.php
    @@ -0,0 +1,63 @@
    +    $log = $options + array(
    

    We can use [] syntax.

  3. +++ b/core/modules/dblog/tests/src/Functional/FakeLogEntries.php
    @@ -0,0 +1,63 @@
    +      'variables'   => array(),
    

    We can use new [] syntax.

  4. +++ b/core/modules/dblog/tests/src/Functional/FakeLogEntries.php
    @@ -0,0 +1,63 @@
    +      'user'        => $this->adminUser,
    +      'uid'         => $this->adminUser->id(),
    

    I think could be safe to check if $this->adminUser is defined before try to use is. Maybe using a ternary operator and fallback to an anonymous user if empty.

ApacheEx’s picture

Thanks for your review. Yeah, I agree with fixes, here is updated patch.

dagmar’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. Thanks @ApacheEx

xjm’s picture

Status: Reviewed & tested by the community » Needs work

As @dawehner said in #9 it's not a good idea to mix moving code and improving it in the same issue. Let's make this issue just about moving the code, and improve it in followups. Thanks!

ApacheEx’s picture

Status: Needs work » Needs review

@xjm,
Perhaps #18 is what we need, is not it?

dagmar’s picture

Status: Needs review » Needs work

I think @xjm refers to #19.4.

Ok, so let's do this, just move the code, inside a trait. Forget all the enhancements, I will create the follow ups. Also, please consider that the [] syntax is already in place.

ApacheEx’s picture

This is just about moving a code and nothing else. All other things like in #20 will be done in follow ups.

p.s Hopefully @dagmar will create a needed follow up (s).

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.

dagmar’s picture

Status: Needs review » Reviewed & tested by the community

Now that this is just moving code around as requested by @dawehner and @xjm I think is good to go.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Ah thanks @dagmar for getting this moving again.

Do you want to create a followup issue for your earlier review points? There are a couple things I will add to it if so. :) The issue can just be RTBC again once the followup is filed.

Thanks!

dagmar’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @xjm

Here is the follow up: #2985701: Followup: Move DbLogTest::generateLogEntries() into a Trait. I'm not sure if we should make the trait dependent of ContainerAwareTrait or just use Drupal::service to address item 1. Could you provide some feedback on that?

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.

  • catch committed ce816c7 on 8.7.x
    Issue #2848914 by pk188, ApacheEx, dagmar: Move DbLogTest::...

  • catch committed 03a8db5 on 8.6.x
    Issue #2848914 by pk188, ApacheEx, dagmar: Move DbLogTest::...
catch’s picture

Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

This is purely internal test re-organisation so fine during the rc. Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!

Status: Fixed » Closed (fixed)

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