Problem/Motivation

HandlerArgumentUserUidTest makes no HTTP requests but is a functional test

Proposed resolution

Convert HandlerArgumentUserUidTest into a Kernel test

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#4 3042472-4.interdiff.txt576 bytesclaudiu.cristea
#4 3042472-4.patch4.93 KBclaudiu.cristea
#2 3042472-2.patch4.93 KBclaudiu.cristea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Issue summary: View changes
Status: Active » Needs review
FileSize
3042472-2.patch4.93 KB

This patch decreases the locally test run from 9.5 to 2.4 seconds.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work

This patch decreases the locally test run from 9.5 to 2.4 seconds.

Nice! It'll be nice to see all these issues land and hopefully knock down the time it takes to run the full core test suite :)

  1. +++ b/core/modules/user/tests/modules/user_test_views/test_views/views.view.test_user_uid_argument.yml
@@ -9,7 +9,7 @@ module: views
-base_field: nid
+base_field: uid

    Interesting. I can take a guess at why this change was made, but how did this test ever work before? :) 

  2. +++ b/core/modules/user/tests/src/Kernel/Views/HandlerArgumentUserUidTest.php
@@ -0,0 +1,77 @@
+  public static $modules = [

    This should be protected. 

  3. +++ b/core/modules/user/tests/src/Kernel/Views/HandlerArgumentUserUidTest.php
@@ -0,0 +1,77 @@
+    $this->installSchema('system', ['sequences']);
+    $this->installEntitySchema('user');
+    $this->installConfig(['user']);
+    User::create(['uid' => 0, 'name' => ''])->save();
+    ViewTestData::createTestViews(static::class, ['user_test_views']);

    This seems like it should go into setUp(). 

  4. +++ b/core/modules/user/tests/src/Kernel/Views/HandlerArgumentUserUidTest.php
@@ -0,0 +1,77 @@
+    $this->assertEquals($account->label(), $view->getTitle());

    If assertEquals() is comparing strings, it should probably be assertSame(). :)

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Needs work » Needs review
FileSize
3042472-4.patch4.93 KB
3042472-4.interdiff.txt576 bytes

@phenaproxima, thank you.

#3.1: That is a bug and is not related directly to this particular conversion, just that is the testing view used here. The kernel test also passes w/o that change. It's a little bit out-of-scope but as I've already discovered, I think it would be too much to fix in a dedicated follow-up.

#3.2: Fixed.

#3.3: I disagree with this. Normally we put in setup code that is reusable across all tests from a class. It's correct to put those lines in ::setUp() but is also correct to let them in the test, when there is only one test or when they concern only that test.

#3.4: Can you elaborate on this, because I think exactly the opposite? And moreover, when we compare strings in Drupal, when one string could be a MarkupInterface object and the other just a string. In that case the assertion will fail. So, we need == (which is assertEquals()), not === (assertSame()).

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented

Can you elaborate on this, because I think exactly the opposite?

If comparing a string to something "stringable", like a MarkupInterface, then you are absolutely right; assertSame() is not the right tool. But if you're asserting the output of a method that returns a plain string, I feel like assertSame() is better, since it is a slightly stronger guarantee that you're asserting a correct value.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at ezCompany commented
Title: Convert HandlerArgumentUserUidTest into a kernel test » Convert HandlerArgumentUserUidTest into a kernel test and clean up the surrounding code
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/user/tests/modules/user_test_views/test_views/views.view.test_user_uid_argument.yml
@@ -9,7 +9,7 @@ module: views
-base_field: nid
+base_field: uid

Yeah, bit of an unrelated change, but not THAT unrelated, since this view is used in the test, and this is small enough in scope to do 'convert and clean up' I think. Updated the title to indicate this

I have no strong feelings on the assertSame vs assertEquals, this is fine with me, so gonna RTBC it, and we'll see...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 9676337962 to 8.8.x and 0b3d688d26 to 8.7.x. Thanks!

As a test only change backported to 8.7.x - I discussed backporting test-only changes with @catch (as a release manager).

I have no strong feelings either on assertEquals vs assertSame

  • alexpott committed 9676337 on 8.8.x 
    Issue #3042472 by claudiu.cristea, phenaproxima, Lendude: Convert...

  • alexpott committed 0b3d688 on 8.7.x 
    Issue #3042472 by claudiu.cristea, phenaproxima, Lendude: Convert...

Status: Fixed » Closed (fixed)

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