Members fund testing for the Drupal project. Drupal Association Learn more

Comments

michielnugter created an issue. See original summary.

Ujin’s picture

Ujin’s picture

Status: Active » Needs review
Ujin’s picture

The last submitted patch, 2: browsertest-menu_ui-2870452-1.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: browsertest-menu_ui-2870452-2.patch, failed testing.

dawehner’s picture

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.

michielnugter’s picture

Component: phpunit » menu_ui.module
Issue tags: +phpunit initiative
Ujin’s picture

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!

dawehner’s picture

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

andypost’s picture

Inlined assertCacheContext

Status: Needs review » Needs work

The last submitted patch, 11: 2870452-menu_ui-11.patch, failed testing.

andypost’s picture

andypost’s picture

andypost’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
4.26 KB

Another try

Status: Needs review » Needs work

The last submitted patch, 15: 2870452-menu_ui-15.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
5.86 KB

And another one

Status: Needs review » Needs work

The last submitted patch, 17: 2870452-menu_ui-17.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.43 KB
7.01 KB

Emulate drupalPost()

Status: Needs review » Needs work

The last submitted patch, 19: 2870452-menu_ui-19.patch, failed testing.

michielnugter’s picture

Quick review to see what's going on here.

  1. +++ b/core/modules/menu_ui/tests/src/Functional/MenuTest.php
    @@ -577,9 +578,16 @@ public function testBlockContextualLinks() {
    -    $response = $this->drupalPost('contextual/render', 'application/json', $post, ['query' => ['destination' => 'test-page']]);
    -    $this->assertResponse(200);
    -    $json = Json::decode($response);
    +    $client = \Drupal::httpClient();
    +    $url = $this->buildUrl('contextual/render', ['query' => ['destination' => 'test-page']]);
    +    $response = $client->post($url, [
    +      'form_params' => $post,
    +      'headers' => [
    +        'Accept' => 'application/json',
    +      ],
    +    ]);
    +    $this->assertEquals(200, $response->getStatusCode());
    +    $json = Json::decode((string) $response);
         $this->assertIdentical($json[$id], '<ul class="contextual-links"><li class="block-configure"><a href="' . base_path() . 'admin/structure/block/manage/' . $block->id() . '">Configure block</a></li><li class="entitymenuedit-form"><a href="' . base_path() . 'admin/structure/menu/manage/' . $custom_menu->id() . '">Edit menu</a></li></ul>');
    

    As it's actually testing Javascript things, shouldn't we move it to a JavascriptTestBase test?

  2. +++ b/core/modules/menu_ui/tests/src/Functional/MenuTest.php
    @@ -867,13 +875,13 @@ public function testMenuParentsJsAccess() {
    -    $this->drupalGetAjax('admin/structure/menu/parents');
    +    $this->drupalGet('admin/structure/menu/parents', ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']], ['X-Requested-With: XMLHttpRequest']);
    ...
    -    $this->drupalGetAjax('admin/structure/menu/parents');
    +    $this->drupalGet('admin/structure/menu/parents', ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']], ['X-Requested-With: XMLHttpRequest']);
    

    Same for these changes.

andypost’s picture

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

naveenvalecha’s picture

Title: Convert web tests to browser tests for menu_ui module » [PP-1] Convert web tests to browser tests for menu_ui module
Issue summary: View changes
Status: Needs work » Postponed
dawehner’s picture

Title: [PP-1] Convert web tests to browser tests for menu_ui module » Convert web tests to browser tests for menu_ui module
Status: Postponed » Needs review

This is in, yeah!

dawehner’s picture

Status: Needs review » Needs work

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
13.36 KB
18.07 KB

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

Status: Needs review » Needs work

The last submitted patch, 27: 2870452-27.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
29.6 KB
34.29 KB

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.

claudiu.cristea’s picture

Unseles modules are enabled in the JTB test.

claudiu.cristea’s picture

Adding @trigger_error() to the deprecated base class.

dawehner’s picture

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.

claudiu.cristea’s picture

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.

Lendude’s picture

Status: Needs review » Needs work

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()

andypost’s picture

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

Lendude’s picture

Status: Needs review » Needs work

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

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.