Problem/Motivation

This is a result of comments made by Kristen Pol in a review of #3200534: Use dataprovider for constructor test in ContentEntityTest mostly related to adding comments. However, one comment pointed out that it wasn't clear why the anonymous user was created. And even worse, the tests still pass when removing anonymous and it should not.

Steps to reproduce

Proposed resolution

Add comments as per #3200534-8: Use dataprovider for constructor test in ContentEntityTest
Add an assertion that the anonymous user exists in testUser.

Remaining tasks

Review
Commit

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#6 3229734-6.patch2.02 KBquietone
#6 interdiff-2-6.txt1.31 KBquietone
#2 3229734-2.patch2.1 KBquietone
#2 3229734-2-fail.patch1.99 KBquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
StatusFileSize
new 3229734-2-fail.patch1.99 KB
new 3229734-2.patch2.1 KB

Here is a patch and a fail patch that shows that the anonymous user is needed.

quietone’s picture

quietone commented
Status: Active » Needs review

The last submitted patch, 2: 3229734-2-fail.patch, failed testing. View results

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs review » Needs work 
+++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/ContentEntityTest.php
@@ -279,6 +282,14 @@ public function testUserSource(array $configuration) {
+      // Confirm that the anonymous user is in the source database.
+      $database = $this->container->get('database');
+      $users = $database->select('users', 'u')
+        ->fields('u')
+        ->condition('u.uid', '0')
+        ->execute()
+        ->fetchAll();
+      $this->assertCount(1, $users);

Is there a reason that the above would be prefered over something like this:

$this->assertNotNull(User::load(0), 'There is not an anonymous user.');

I would rather see a single line if there's not some subtlety I'm missing.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2-6.txt1.31 KB
new 3229734-6.patch2.02 KB

@danflanagan8, thanks!

#5. Sure, one line is nicer. And I merged the two comments into one because I thought it read better.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs review » Reviewed & tested by the community

I agree that the updated comment is much clearer. This looks good to me.

Edit: For the record, I confirmed that the one-line assertion fails when the anonymous user is not created in setUp. I suppose we could update the fail patch with the one-line assertion and run it on d.o. I did that locally though and it went as expected.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 3229734-6.patch, failed testing. View results

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented
Status: Needs work » Reviewed & tested by the community
Related issues: +#3099427: [random test failure] FieldLayoutTest::testEntityView()

That's an unrelated random test failure. Looks like #3099427: [random test failure] FieldLayoutTest::testEntityView(). Resetting to RTBC.

alexpott credited Kristen Pol.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Crediting @Kristen Pol since many of these changes are due to a comment on the other issue.

Committed and pushed 0bed3aeeef to 9.3.x and 67a30ac667 to 9.2.x. Thanks!

Backported to 9.2.x as these are test-only changes.

  • alexpott committed 0bed3ae on 9.3.x 
    Issue #3229734 by quietone, danflanagan8, Kristen Pol: Improve test and...

  • alexpott committed 67a30ac on 9.2.x 
    Issue #3229734 by quietone, danflanagan8, Kristen Pol: Improve test and...

Status: Fixed » Closed (fixed)

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