BrowserTestBase::drupalCreateUser() should use UserCreationTrait::createUser()

Great job, simplifies the code base and leverages an existing trait.

+++ b/core/modules/simpletest/src/UserCreationTrait.php
@@ -77,6 +77,8 @@ protected function createUser(array $permissions = array(), $name = NULL, $admin
+    // Support BrowserTestBase as well.
+    $account->passRaw = $account->pass_raw;

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -56,6 +57,10 @@
 

This is not the most elegant thing, but for BC reasons it's fine to do it like this. We cannot rename this property any more at this point.

This is not the most elegant thing, but for BC reasons it's fine to do it like this. We cannot rename this property any more at this point.

Could we add $passRaw to the testing frameworks and deprecate $pass_raw? Since our coding standards do say class members should be camelCase. That could be a followup; not in scope here. Then we could simply remove the old, inconsistently named properties in 9.x.

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -56,6 +57,10 @@
+  use UserCreationTrait {
+    createRole as drupalCreateRole;
+    createUser as drupalCreateUser;
+  }

This is also something that might benefit from a followup for method addition/deprecation to later rename these (for both web and browser tests). (Could be part of the same followup.)

I read over the removed code and confirmed it's pretty much pure duplicated code. Diffstats like this make me happy! And it's completely BC as a bonus. Nice work.

Committed c572b92 and pushed to 8.2.x. Thanks!

Removed now-unused use statements on commit:

diff --git a/core/tests/Drupal/Tests/BrowserTestBase.php b/core/tests/Drupal/Tests/BrowserTestBase.php
index 4a8ac3b..1da673a 100644
--- a/core/tests/Drupal/Tests/BrowserTestBase.php
+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -28,8 +28,6 @@
 use Drupal\simpletest\BlockCreationTrait;
 use Drupal\simpletest\NodeCreationTrait;
 use Drupal\simpletest\UserCreationTrait;
-use Drupal\user\Entity\Role;
-use Drupal\user\Entity\User;
 use Symfony\Component\CssSelector\CssSelectorConverter;
 use Symfony\Component\HttpFoundation\Request;

Can we have this in 8.1.x just like we did for the previous BrowserTestBase changes? The touched files are identical in 8.2.x and 8.1.x (core/tests/Drupal/Tests/BrowserTestBase.php and core/modules/simpletest/src/UserCreationTrait.php)

Because everything is identical git cherry-pick c572b92 -X theirs does the job and is as effective as resolving the conflict. (git cherry-pick c572b92 doesn't automatically resolve the "conflict". BrowserTestBase has followed a different route in 8.1.x than in 8.2.x. A couple of the earlier commits weren't cherry-picked but applied separately.)

Therefor committing this one with git cherry-pick c572b92 -X theirs is preferable to doing a completely separate commit.

Here is a patch created after git cherry-picking!

As noted in #9: the touched files are actually identical in 8.1.x.
To avoid possible future BrowserTestBase backport problems with git cherry-pick it would certainly help if core committers would actually cherry-pick this one, either with conflict resolution or in this special case with git cherry-pick c572b92 -X theirs.

Thanks, @jhedstrom for pointing out some stuff.

Hmm, guess I missed something earlier when i said the touched files were identical.

8.2.x - 2758067-02.patch:
- $assigned_permissions = Role::load($role->id())->getPermissions();

8.1.x - 2758067-12.patch:
- $assigned_permissions = entity_load('user_role', $role->id())->getPermissions();

The git cherry-pick c572b92 -X theirs trick worked out fine here, bit my earlier reasoning doesn't apply.

What happens when an existing subclass uses this trait already. Doesn't that cause some issues? Nope, it doesn't see https://3v4l.org/lsIgN

Given that we continue to work on those tests issues having something more in sync is not a bad idea in the first place.

So I'm not sure about backporting this change to 8.1.x. If it were production code, I probably would not consider it, despite @dawehner's reassurance about it being non-disruptive above. Our allowed changes policy considers internal refactoring (for which this might qualify) for patch releases at committer discretion only.

Other improvements intended to make the test conversions easier have been going into 8.2.x only, so the branches have already diverged. What is the case for backporting this?

Also filed: #2773087: [policy, maybe patch] Clarify backport policy for testing framework changes, including new BTB tests

Other improvements intended to make the test conversions easier have been going into 8.2.x only, so the branches have already diverged.

At this moment only the last two changes to BrowserTestBase did not make it into 8.1.x. The entity_load() conversion (unrelated to test conversion) was responsible for divergence. But if we backport here that implementation divergence will be moved into a trait, which would be really nice.

Based on git diff 8.2.x -- core/tests/Drupal/Tests/BrowserTestBase.php as well as feedback from @Eric_A, @dawehner, and @Berdir on #2773087: [policy, maybe patch] Clarify backport policy for testing framework changes, including new BTB tests, I will backport this to 8.1.x. However, none of the suggested cherry-picks or uploaded patches cleanly fixed the use statement issue I previously resolved, so to be on the safe side, here's a clean backport patch so we can ensure tests pass with it.

Committed 4775a28 and pushed to 8.1.x. Thanks!