Problem/Motivation

We want to convert all old Simpletest web tests to BrowserTestBase, see #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase). BrowserTestBase currently does not work with the standard installation profile.

Proposed resolution

Let's convert StandardTest to BrowserTestBase, which tests the standard installation profile.

Therefore we need:

  • Ensure that shutdown functions are registered correctly
  • Introduce a AssertLegacyTrait, which does some stuff, same as KernelTestBase does.
  • Expand the available methods on the WebAssert class
  • Adapt assertEquals() to support MarkupInterface objects

Remaining tasks

Review patch.

User interface changes

None.

API changes

Only API additions to BrowserTestBase.

Data model changes

None.

CommentFileSizeAuthor
#40 interdiff.txt580 bytesklausi
#40 browser-standard-2735045-40.patch24.53 KBklausi
#34 interdiff.txt848 bytesdawehner
#34 2735045-34.patch24.52 KBdawehner
#31 interdiff.txt857 bytesdawehner
#31 2735045-31.patch24.51 KBdawehner
#29 interdiff.txt9.96 KBdawehner
#29 2735045-29.patch24.91 KBdawehner
#22 interdiff.txt13.32 KBdawehner
#22 2735045-22.patch24.69 KBdawehner
#21 interdiff.txt1.1 KBklausi
#21 browser-standard-2735045-21.patch22.27 KBklausi
#19 interdiff.txt2.23 KBklausi
#19 browser-standard-2735045-19.patch22.17 KBklausi
#17 interdiff.txt16.67 KBklausi
#17 browser-standard-2735045-17.patch21.26 KBklausi
#13 browsertestbase_does-2735045-13.patch14.5 KBjibran
#13 interdiff.txt6.13 KBjibran
#10 browsertestbase_does-2735045-10.patch10.84 KBjibran
#10 interdiff.txt10.32 KBjibran
#8 interdiff.txt16.99 KBklausi
#8 browser-standard-2735045-8.patch17.28 KBklausi
#4 interdiff.txt1.88 KBklausi
#4 browser-standard-2735045-4.patch2.11 KBklausi
#2 browsertest-standard-2735045-2.patch788 bytesklausi
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Active » Needs review
FileSize
788 bytes

Here is a failing test.

Status: Needs review » Needs work

The last submitted patch, 2: browsertest-standard-2735045-2.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
2.11 KB
1.88 KB

Found the problem, we need to handle shut down callbacks same as TestBase does.

dawehner’s picture

Do we need the same kind of code in KernelTestBase?

jibran’s picture

+++ b/core/tests/Drupal/FunctionalTests/BrowserTest/StandardProfileTest.php
@@ -20,7 +20,7 @@ class StandardProfileTest extends BrowserTestBase {
+    $this->assertTrue(TRUE);

Let's visit front page and check the blocks to make sure that we have valid html.

jibran’s picture

Or maybe we can convert \Drupal\standard\Tests\StandardTest to BTB.
This test should be moved to \Drupal\Tests\standard\Functional namespace.

klausi’s picture

@dawehner: not sure, as long as we don't run into problems I think we can ignore it for kernel tests.

@jibran: good idea, although converting StandardTest requires porting tons of assertion functions. Not fully done with StandardTest, so this will fail.

Status: Needs review » Needs work

The last submitted patch, 8: browser-standard-2735045-8.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
10.32 KB
10.84 KB

How about this? interdiff is from #4.

jibran’s picture

+++ b/core/profiles/standard/tests/src/Functional/StandardProfileTest.php
@@ -90,19 +94,21 @@ function testStandard() {
-    $this->assertRaw('Then she picked out two somebodies,<br />Sally and me', 'Found a line break.');
...
+    $page = $this->getSession()->getPage();
+    $this->assertContains('Then she picked out two somebodies,<br />Sally and me', $page->getHtml(), 'Found a line break.');

I think we should add a helper method for assertRaw to assert session.

Status: Needs review » Needs work

The last submitted patch, 10: browsertestbase_does-2735045-10.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
6.13 KB
14.5 KB
jibran’s picture

+++ b/core/modules/system/config/schema/system.schema.yml
@@ -279,6 +279,9 @@ system.file:
+        private:
+          type: string
+          label: 'Private directory'

Found a bug.

Status: Needs review » Needs work

The last submitted patch, 13: browsertestbase_does-2735045-13.patch, failed testing.

jibran’s picture

+++ b/core/profiles/standard/tests/src/Functional/StandardProfileTest.php
@@ -148,32 +159,34 @@ function testStandard() {
+    $this->drupalLogin($root_user);

I don't know why root user is not able to login.

klausi’s picture

Status: Needs work » Needs review
FileSize
21.26 KB
16.67 KB

Thanks jibran, but I think we should modify StandardTest as little as possible and provide maximum method compatibility on BrowserTestBase. That will make conversions easier since we don't have to change the actual test code as much (as first step).

I moved all the assertion helper methods to a trait.

StandardTest is now passing without test code changes, hooray!

Interdiff is against my previous patch, not jibran's.

klausi’s picture

klausi’s picture

Turns out that BrowserTestBase is wrong in this case, not the config schema. Copied the code from WebTestBase and improved comments.

klausi’s picture

Private files were moved to settings.php in #2170235: file_private_path should be in $settings, like file_public_path, looks like that got lost when BrowserTestBase was created or we forgot to update.

klausi’s picture

Fixed use statements discovered when working on #2735199: Convert web tests to browser tests for help module.

dawehner’s picture

I left a big review in https://www.drupal.org/node/2735199. Here is a start of that:

  • Provide good named methods in WebAssert
  • Introduce a LegacyTrait to make it easy to convert old methods
klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -0,0 +1,182 @@
    + * @deprecated Scheduled for removal in Drupal 9.0.0. Use the wrapped methods
    + * instead.
    

    Should be indented 2 spaces on the second line. What are the wrapped methods you are referring to? I think the comment should be "Use methods of the WebAssert class instead, for example @code $this->assertSession()->statusCodeEquals(200); @endcode".

  2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -0,0 +1,182 @@
    +  public function assertSession($name = NULL) {
    +    return new WebAssert($this->getSession($name));
    +  }
    

    this method is not deprecated, so it should not be on the legacy trait.

  3. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -0,0 +1,182 @@
    +   *   Use $assert->elementExists() instead.
    

    I think this should be $this->assertSession()->elementExists(), because people don't know where $assert is coming from.

  4. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -0,0 +1,182 @@
    +  protected function assertElementPresent($css_selector, $message = '') {
    

    $message is unused here and should be removed.

  5. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
    @@ -0,0 +1,182 @@
    +  protected function assertElementNotPresent($css_selector, $message = '') {
    

    $message is unused here and should be removed.

  6. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1529,4 +1608,153 @@ protected function cssSelectToXpath($selector, $html = TRUE, $prefix = 'descenda
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function assertEquals($expected, $actual, $message = '', $delta = 0.0, $maxDepth = 10, $canonicalize = FALSE, $ignoreCase = FALSE) {
    +    $expected = static::castSafeStrings($expected);
    +    $actual = static::castSafeStrings($actual);
    

    this should have a comment why we override the parent method. The more I think of it the more I don't like that we override assertEquals like that. It changes the meaning of the assert. Instead, I think we should fix all the invocations where we convert to string beforehand.

  7. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -64,4 +66,72 @@ public function selectExists($select, TraversableElement $container = NULL) {
    +  /**
    +   * Asserts a condition.
    +   *
    +   * @param bool   $condition
    +   * @param string $message   Failure message
    +   *
    +   * @throws \Behat\Mink\Exception\ExpectationException
    +   *   when the condition is not fulfilled.
    +   */
    +  public function assert($condition, $message) {
    

    why is that overridden from the parent class? Please add a comment.

klausi’s picture

  1. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -1529,4 +1608,153 @@ protected function cssSelectToXpath($selector, $html = TRUE, $prefix = 'descenda
    +  protected function config($name) {
    +    return \Drupal::configFactory()->getEditable($name);
    +  }
    

    does not bleong on the legacy assert trait.

  2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    --- a/core/tests/Drupal/Tests/WebAssert.php
    +++ b/core/tests/Drupal/Tests/WebAssert.php
    

    Let's keep this patch small by only adding the legacy assertion trait and not making any modifications to our Webassert class, we don't need it for this particular issue where we just fix StandardTest.

dawehner’s picture

@klausi
Do you agree with the general idea?

klausi’s picture

yes, I agree that we can expand WebAssert.php with useful helper methods. Thanks for your work on that and the idea! Can you move it to a separate issue?

dawehner’s picture

@klausi
Thank you for your review btw.

Mh, so you would commit your latest commit as it is. To be honest I'm not sure which of the conversions actually is kinda the root, aka. the one which introduces the BrowserAssertTrait. Its tricky to introduce something, as people might use it, when we remove it again.

klausi’s picture

I think this issue should introduce the assertion trait - and I like your name LegacyAssertTrait a lot more, because we should really be deprecating those assert methods and rely more on mink standard assertions. We don't have to maintain those, which means less code and less mistakes we can make.

dawehner’s picture

Status: Needs work » Needs review
FileSize
24.91 KB
9.96 KB

Should be indented 2 spaces on the second line. What are the wrapped methods you are referring to? I think the comment should be "Use methods of the WebAssert class instead, for example @code $this->assertSession()->statusCodeEquals(200); @endcode".

Great improvement!

this should have a comment why we override the parent method. The more I think of it the more I don't like that we override assertEquals like that. It changes the meaning of the assert. Instead, I think we should fix all the invocations where we convert to string beforehand.

I hope its okay to add the documentation to KernelTestBase as well, where I got the code from.

why is that overridden from the parent class? Please add a comment.

* The parent method is overridden because its a private method.
:(

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1529,4 +1608,153 @@ protected function cssSelectToXpath($selector, $html = TRUE, $prefix = 'descenda
+ protected function config($name) {
+ return \Drupal::configFactory()->getEditable($name);
+ }
does not bleong on the legacy assert trait.

It already isn't, see your bit.

klausi’s picture

Status: Needs review » Needs work

Almost ready, this looks fluffy good!

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -0,0 +1,164 @@
+  protected function pass($message = '') {
+  }

pass() should not be empty, it should have an always true assertion. Otherwise PHPUnit will complain about a risky test if the test does not contain any other assertion.

I wonder if we should deprecate the usage of pass() as well. Since we are unsure and you did not deprecate this method let's move it to BrowserTestBase.

dawehner’s picture

Status: Needs work » Needs review
FileSize
24.51 KB
857 bytes

Turns out, the other legacy trait already has the pass method.

klausi’s picture

Status: Needs review » Needs work

Great, another round of nitpicks now that we have to keep the assert() method.

  1. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -64,4 +66,83 @@ public function selectExists($select, TraversableElement $container = NULL) {
    +   * The parent method is overridden because its a private method.
    

    "its" should be "it is"

  2. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -64,4 +66,83 @@ public function selectExists($select, TraversableElement $container = NULL) {
    +   * @param bool $condition
    +   *   The condition
    +   * @param string $message
    +   *   The failure message
    

    @param description should end with dots.

  3. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -64,4 +66,83 @@ public function selectExists($select, TraversableElement $container = NULL) {
    +   * @throws \Behat\Mink\Exception\ExpectationException
    +   *   when the condition is not fulfilled.
    

    Description should start with a capital letter.

klausi’s picture

+++ b/core/tests/Drupal/Tests/WebAssert.php
@@ -64,4 +66,83 @@ public function selectExists($select, TraversableElement $container = NULL) {
+   * @param string $message
+   *   The failure message

This is not a failure message, it is an assertion message. "The assertion message describing the condition."

dawehner’s picture

Status: Needs work » Needs review
FileSize
24.52 KB
848 bytes

Thank you!

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, assuming the testbot agrees.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

So this is doing way more than the issue title suggests - it's expanding BrowserTestBase's surface area to include legacy assertions. I think this is a good idea and will make conversion go much quicker - as shown by the conversion of StandardTest - but we need to be honest about what the issue is doing and why.

alexpott’s picture

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1529,4 +1621,156 @@ protected function cssSelectToXpath($selector, $html = TRUE, $prefix = 'descenda
+    return \Drupal::configFactory()->getEditable($name);

Let's go for $this->container here. More future proof and encourages us to always make sure the container is up to date.

dawehner’s picture

Title: BrowserTestBase does not work with standard profile » Convert StandardTest to BrowserTestBase by introducing a AssertLegacyTrait and new assertions on the WebAssert

Better title first

dawehner’s picture

Issue summary: View changes
klausi’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
24.53 KB
580 bytes

Removed the \Drupal usage in the new config() method, improved issue summary.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you klausi!

klausi’s picture

Just hiding some files.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0d9a9c9 and pushed to 8.1.x and 8.2.x as this is opens up the possibility of converting tests to BrowserTestBase easily and divergence in 8.1.x and 8.2.x test infra is undesirable. Thanks!

  • alexpott committed a4bf1e9 on 8.2.x
    Issue #2735045 by klausi, dawehner, jibran: Convert StandardTest to...

  • alexpott committed 0d9a9c9 on 8.1.x
    Issue #2735045 by klausi, dawehner, jibran: Convert StandardTest to...
pfrenssen’s picture

This is causing the BrowserTestBase tests in OG to fail, seemingly because the assertLink() method is public instead of protected. Created followup: #2744089: Fix visibility of AssertLegacyTrait::assertLink().

neclimdul’s picture

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -1241,6 +1349,14 @@ protected function prepareEnvironment() {
@@ -1402,13 +1518,13 @@ protected function refreshVariables() {

@@ -1402,13 +1518,13 @@ protected function refreshVariables() {
   /**
    * Returns whether a given user account is logged in.
    *
-   * @param \Drupal\user\UserInterface $account
+   * @param \Drupal\Core\Session\AccountInterface $account
    *   The user account object to check.
    *
    * @return bool
    *   Return TRUE if the user is logged in, FALSE otherwise.
    */
-  protected function drupalUserIsLoggedIn(UserInterface $account) {
+  protected function drupalUserIsLoggedIn(AccountInterface $account) {
     $logged_in = FALSE;
 
     if (isset($account->sessionId)) {

Can I get a quick explanation of why this was changed as part of this issue so I can make the correct fix to #2735235: BrowserTestBase standards cleanup. I don't see any discussion and I don't see any uses of it or ::drupalLogin in the patch so its not clear.

klausi’s picture

We need AccountInterface because the following line should work:

$this->drupalLogin($this->rootUser);

$this->rootUser is created as UserSession in installDrupal().

klausi’s picture

And $this->drupalLogin($this->rootUser); usage was discovered in #2735199: Convert web tests to browser tests for help module and ported to this issue.

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

Issue tags: +8.2.0 release notes

Even though this was also cherry picked to 8.1, I think it's worth mentioning in the 8.2.0 release notes.