Convert web tests to browser tests for menu_ui module

Hi @Ujin, thank you for your contribution!

First tip: Try to use the following git configuration:

[diff]
renames = copies
This detects renames, see https://www.drupal.org/documentation/git/configure for more details. This will make the patch size significantly smaller.

1) Drupal\Tests\menu_ui\Functional\MenuNodeTest::testMenuNodeFormWidget
PHPUnit_Framework_Exception: Fatal error: Call to undefined method Drupal\Tests\menu_ui\Functional\MenuNodeTest::assertCacheContext() in /var/www/html/core/modules/menu_ui/tests/src/Functional/MenuNodeTest.php on line 60

This requires #2795085: Add assertNoCacheTag to assertLegacyTrait to be in. Note: you could convert the function calls manually.

Hi @dawehner,
Thank you for the help, I am new to Drupal 8 testing, I am testing using this line
php core/scripts/run-tests.sh --verbose --browser chrome --url http://my-url/ --class --color "\Drupal\Tests\menu_ui\Functional\MenuTest"
and see errors like this:

Drupal\Tests\menu_ui\Functional\MenuTest::testMenu
Exception: Warning: mkdir(): Permission denied
Drupal\Component\PhpStorage\FileStorage->createDirectory()() (Line: 157)

What could be the issue ?
Thanks!

You probably run the tests with the wrong user, see the documentation on https://www.drupal.org/docs/8/phpunit/running-phpunit-tests

Inlined assertCacheContext

Another try

And another one

Emulate drupalPost()

Yep, both cases means that tests needs split some how (maybe on per method basis)

This issue is blocked by #2795085: Add assertNoCacheTag to assertLegacyTrait

This is in, yeah!

Applied suggestions from #21 - created FunctionalJavascript/MenuWebTestBase and FunctionalJavascript/MenuTest to contain testBlockContextualLinks() and testMenuParentsJsAccess () from Functional/MenuTest.

Few points:

• \Drupal\menu_ui\Tests\MenuWebTestBase should not be removed as is a base class and it's possible that some contribs are extending it. Instead it should be deprecated.
• MenuWebTestBase has only a method, ::assertMenuLink() and that is used in the new BTB and new JTB classes. Thus we should not create a base class, neither for BTB, nor for JTB new classes but we should move ::assertMenuLink()
  in a trait because is pure code reusing between BTB and JTB.
• The ::assertMenuLink() signature parameters should be swapped because in PHPUnit the expectation should come first.
• The classes prefix Menu* is wrong because the module is "Menu UI". So I renamed all the classes that I'm converting to use the MenuUi* prefix.
• The new MenuUiJavascriptTest is using specific UI actions (as contextual links) to perform the test.

I posted an interdiff but I don't think it's too relevant.

Unseles modules are enabled in the JTB test.

Adding @trigger_error() to the deprecated base class.

I'm wondering whether its really the right step to rename test classes, etc. The patch could be way smaller than it actually is right now.

I'm wondering whether its really the right step to rename test classes, etc. The patch could be way smaller than it actually is right now

On all, except one, we're adding use MenuUiTrait; in the proximity of the class statement. So the patch would not decrease to much (only with 2 x 2 lines plus 1 hunk.

Nice work on this one! But pffff hard to read/review diff here.

1. +++ b/core/modules/menu_ui/src/Tests/MenuWebTestBase.php
   @@ -2,13 +2,23 @@
   +@trigger_error('The ' . __NAMESPACE__ . '\MenuWebTestBase is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0. Use the \Drupal\Tests\BrowserTestBase base class and the \Drupal\Tests\menu_ui\Traits\MenuUiTrait trait instead. See https://www.drupal.org/node/2870452.', E_USER_DEPRECATED);
   

   No need for the 'The ' before the __NAMESPACE__

2. +++ b/core/modules/menu_ui/src/Tests/MenuWebTestBase.php
   @@ -2,13 +2,23 @@
    /**
     * Base class for menu web tests.
   + *
   + * @deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.x. Use
   + *   \Drupal\Tests\menu_ui\Traits\MenuUiTrait methods, instead.
     */
   

   The docblock also needs a @see to the CR

3. +++ b/core/modules/menu_ui/src/Tests/MenuWebTestBase.php
   @@ -2,13 +2,23 @@
   +  use MenuUiTrait {
   +    assertMenuLink as phpUnitAssertMenuLink;
   +  }
   

   The fact that this adds something named phpUnitXXXX to a simpletest class should indicate that this might not be the right way to do this. I'd opt to not change the existing base class except for adding the deprecation. Yes this gives us some code duplication for now, but will allow us to do PHPUnit specific code in the new trait without having to worry about Simpletest BC stuff.

4. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -856,25 +846,7 @@ public function enableMenuLink(MenuLinkContent $item) {
   -  public function testMenuParentsJsAccess() {
   
   @@ -939,6 +911,22 @@ private function verifyAccess($response = 200) {
   +  public function testMenuParentsJsAccess() {
   

   This method gets removed and then added back to the same class (it was moved to the JS test, where it was rightly removed from again). But lets not remove it in the first place.

5. +++ b/core/modules/menu_ui/tests/src/FunctionalJavascript/MenuUiJavascriptTest.php
   @@ -0,0 +1,159 @@
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   ...
   +    $this->assertSession()->assertWaitOnAjaxRequest();
   

   Try to avoid assertWaitOnAjaxRequest, try to waitForElement()

Filed CR https://www.drupal.org/node/2917910 as mentioned it

Fixed #34 1,2,4

3) I think better to have it as example showing how to migrate tests instead of code duplication
5) Why avoid it, it explicitly tests & throws exception when ajax goes wrong

Thanks for working on this @andypost!

I think better to have it as example showing how to migrate tests instead of code duplication

Well this example actually clearly illustrates why this doesn't work. When you use \Drupal\menu_ui\Tests\MenuWebTestBase after applying this patch and call assertMenuLink, internally it will call assertEquals which doesn't exist on WebTestBase

+++ b/core/modules/menu_ui/tests/src/Traits/MenuUiTrait.php
@@ -48,29 +40,29 @@ public function assertMenuLink($menu_plugin_id, array $expected_item) {
+      $this->assertEquals($expected_item['langcode'], $entity->langcode->value);

Now there is PHPUnit code inside a Simpletest test => fatal error. So that is exactly why we need a Simpletest version and a PHPUnit version for these things. And yeah this leads to some code duplication, but since one is deprecated we only have to really maintain one version.

Why avoid it, it explicitly tests & throws exception when ajax goes wrong

I was proven to be a really unstable way of testing for AJAX activity and lead to a fair number of random fails, see #2829040: [meta] Known intermittent, random, and environment-specific test failures and some discussion in #2809161: Convert Javascript/AJAX testing to use JavascriptTestBase. It's just better to wait for something specific if you can, it also makes the test more explicit.

edit: fixed the second link

RTBC++

Another approach, is always wait cotextual links inside of clickContextualLink() method.

Nice! Yes much better this way.

All feedback has been addressed, looks good to me now.

+++ b/core/modules/menu_ui/src/Tests/MenuWebTestBase.php
@@ -2,10 +2,17 @@
+@trigger_error(__NAMESPACE__ . '\MenuWebTestBase is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0. Use the \Drupal\Tests\BrowserTestBase base class and the \Drupal\Tests\menu_ui\Traits\MenuUiTrait trait instead. See https://www.drupal.org/node/2870452.', E_USER_DEPRECATED);
...
+ * @deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.x. Use

s/8.5.x/8.6.x

Also the link should point to CR, not to the issue. (https://www.drupal.org/node/2870452 is the issue)

@vaplas, you're right. I see the test conversions are committed to both, 8.5.x and 8.6.x (see #2863626: Convert web tests to browser tests for image module). Still need to change the CR address.

@claudiu.cristea thanks for catching that and @vaplas thanks for fixing it!

Feedback has been addressed, back to RTBC.

Committed and pushed 51afa6813a to 8.6.x and ee7d05c318 to 8.5.x. Thanks!