Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

tim.plunkett’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
16.94 KB

Let's just do these all here, since some of them are interdependent. Please give m1r1k credit as well on this.

Status: Needs review » Needs work

The last submitted patch, 3: account-2073531-3.patch, failed testing.

dawehner’s picture

All this test failures also appeared in #2004086: The Request service must be synthetic

One major problem is that user_access() relies on global $user, but other ones are for example that a test did relied
on WebTestBase->drupalLogin() not changing $GLOBALS['user'], so that global $user is actually user 1(insert a big WTF here).

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs work » Needs review
FileSize
18.38 KB
1.44 KB

Tracked down the comment one. Posting a patch before a meeting :) will follow-up more later

Status: Needs review » Needs work

The last submitted patch, 6: account-2073531-4.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: account-2073531-4.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
33.92 KB
18.88 KB

Trying more things...

Status: Needs review » Needs work

The last submitted patch, 8: account-2073531-5-global-user.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: account-2073531-5.patch, failed testing.

tim.plunkett’s picture

I don't even understand what's happening anymore.
Without replacing every single global $user/$GLOBALS['user'], I can't be sure of what's triggering it.

Also, the theme callback stuff is triggering this pretty early, so that'd be nice to get out of the way...

Status: Needs review » Needs work

The last submitted patch, 11: account-2073531-6.patch, failed testing.

dawehner’s picture

FileSize
3.43 KB
23.08 KB

After literal hours of debugging I realized that $this->web_user is overridden in the CommentNodeAccessTest file. This causes
to save the wrong node author. You might ask how this ever worked? No idea! (beside maybe admin was always the current user).

Next one: Drupal\comment\Tests\CommentTranslationUITest The admin user did not had 'skip comment approval', so the comment got never published.

PS: Small tip: if you run tests via the UI run them at anonymous user, given that this catches some of the bugs that just appear on run-tests.sh

dawehner’s picture

Status: Needs work » Needs review

.

Status: Needs review » Needs work

The last submitted patch, 13: 2073531.patch, failed testing.

dawehner’s picture

FileSize
2.48 KB
24.18 KB

The failure in SearchCommentCountToggleTest.php is caused by a different bug: For some reasons uid = 0 results in a NULL value of $node->get('uid')->value

dawehner’s picture

Status: Needs work » Needs review

.

Status: Needs review » Needs work

The last submitted patch, 16: 2073531.patch, failed testing.

tim.plunkett’s picture

Issue tags: +WSCCI, +MenuSystemRevamp
dawehner’s picture

Yeah I could not find anything after a total of 10hrs.

tim.plunkett’s picture

Issue tags: +D8MI

Because the remaining failures are in content_translation, tagging so someone will help...

xjm’s picture

Issue tags: +beta blocker
Berdir’s picture

Hum, the retranslate flag fails, I've seen those way too often.

I'll try to have a look, @plach is usually good at figuring out what's going on with the translation UI tests, they're nasty.

plach’s picture

Assigned: tim.plunkett » plach

Working on those failures.

Berdir’s picture

Status: Needs work » Needs review
FileSize
25.29 KB
1.12 KB

Ok, don't completely understand what's going on, but here's the problem:

The drupalPostForm() on line 145 results in a validation error about "Anonymous" not being a valid user.

As a workaround, I added a check for uid 0, which probably should be there anyway, but that doesn't really explain why it was like it was before. But the node author form does it in a similar way so I assume it's necessary.

Oh, and if you want to know. Figuring this out had me write 150 lines of debug output in a file and then *diff* a passing and failing run, there I noticed a few missing lines that I could track down a form submit not happening.

Berdir’s picture

Ok. The 0 in there is coming from content_translation_entity_insert(), which runs within the test code. But as I said, my change IMHO makes sense anyway, as 0 *is* a valid value.

Berdir’s picture

FileSize
26.65 KB
2.47 KB

Ok, here's a patch that fixes that function, also removed a now apparently useless rebuildContainer() call that reset the current user service. Tested that it also works without the change in the content translation controller, but keeping that change.

plach’s picture

Assigned: plach » Unassigned

0 *is* a valid value

Definitely :)

Status: Needs review » Needs work

The last submitted patch, 25: 2073531-24.patch, failed testing.

Berdir’s picture

Oh, looks like the rebuildContainer() removal there *did* cause some fails? That's.. not nice.

dawehner’s picture

Oh, and if you want to know. Figuring this out had me write 150 lines of debug output in a file and then *diff* a passing and failing run, there I noticed a few missing lines that I could track down a form submit not happening.

This is a really awesome technique!

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
25.88 KB
598 bytes

I'm leaving the rebuild in. It's really not our problem to fix it here.

The standard profile test fix was really stupid.
When creating the dummy content, the auth user was having input with trimmed whitespace, and anon wasn't. It came down to a global $user in filter_default_format().

This means that in EVERY OTHER USAGE of global $user, our tests aren't good enough. They should all have failed. But that's out of scope.

We need to get this in ASAP.

Status: Needs review » Needs work

The last submitted patch, 32: account-2073531-32.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

32: account-2073531-32.patch queued for re-testing.

dawehner’s picture

This means that in EVERY OTHER USAGE of global $user, our tests aren't good enough. They should all have failed. But that's out of scope.

I remember back in D6 that writing views API tests was hard, as you had to switch the global user. It is great to know
that we know have a proper way to fix it.

disasm’s picture

  1. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTranslationUITest.php
    @@ -155,7 +155,7 @@ protected function assertPublishedStatus() {
    -    $this->admin_user = $this->drupalCreateUser(array_merge(parent::getTranslatorPermissions(), array('access administration pages', 'administer comments')));
    +    $this->admin_user = $this->drupalCreateUser(array_merge(parent::getTranslatorPermissions(), array('access administration pages', 'administer comments', 'skip comment approval')));
    
    +++ b/core/modules/comment/lib/Drupal/comment/Tests/Views/CommentTestBase.php
    @@ -36,7 +36,7 @@ function setUp() {
    -    $this->account = $this->drupalCreateUser();
    +    $this->account = $this->drupalCreateUser(array('skip comment approval'));
    
    +++ b/core/modules/forum/lib/Drupal/forum/Tests/Views/ForumIntegrationTest.php
    @@ -62,6 +62,9 @@ public function testForumIntegration() {
    +    $account = $this->drupalCreateUser(array('skip comment approval'));
    
    +++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessPagerTest.php
    @@ -55,6 +55,7 @@ public function testCommentPager() {
    +        'status' => COMMENT_PUBLISHED,
    
    +++ b/core/modules/user/lib/Drupal/user/Tests/Views/HandlerFieldUserNameTest.php
    @@ -30,6 +30,8 @@ public static function getInfo() {
    +    $this->drupalLogin($this->drupalCreateUser(array('access user profiles')));
    

    What's the reason for these additional permissions being added to these test users?

  2. +++ b/core/modules/content_translation/lib/Drupal/content_translation/ContentTranslationController.php
    @@ -234,7 +234,14 @@ public function entityFormAlter(array &$form, array &$form_state, EntityInterfac
    +      // Default to the anonymous user.
    +      $name = '';
    +      if ($new_translation) {
    +        $name = $GLOBALS['user']->getUsername();
    +      }
    +      elseif ($entity->translation[$form_langcode]['uid']) {
    +        $name = user_load($entity->translation[$form_langcode]['uid'])->getUsername();
    +      }
    

    This seems backwards to me. We're switching the Controller to go back to GLOBALS['user']?

tim.plunkett’s picture

1) The entirety of this bug is that global $user is actually the user running the test, and it was only working because it thought it was uid 1. Now that it's not, we need to give them the proper perms.

2) We're leaving that $GLOBALS['user'] as is, and just fixing the logic around it.

disasm’s picture

Status: Needs review » Reviewed & tested by the community

Marking RTBC. Fine with both explanations. Lets make sure we get a follow-up opened though for fixing GLOBAL $users everywhere (possibly a meta).

andypost’s picture

+1 to RTBC here!
Let's get this one asap and unfreeze all conversions!

andypost’s picture

catch’s picture

file_put_contents() and diff is also my favourite last-resort debugging technique, never fails (yet).

Committed/pushed to 8.x, thanks!

catch’s picture

Status: Reviewed & tested by the community » Fixed
damiankloip’s picture

Status: Fixed » Closed (fixed)

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