Comments

naveenvalecha created an issue. See original summary.

Lendude’s picture

Title: Convert AssertBreadcrumbTrait from WTB to BTB » Menu: convert system functional tests to PHPUnit
Issue summary: View changes
Related issues: +#2862508: Convert system functional tests to phpunit

This should be done as a part of the System modules Menu tests conversion. There was no issue for that yet, so lets use this one.

naveenvalecha’s picture

Here's the initial patch. Let's see how many fails.

naveenvalecha’s picture

Status: Active » Needs review
naveenvalecha’s picture

Issue summary: View changes

Updating IS

Status: Needs review » Needs work

The last submitted patch, 3: 2887099-3.patch, failed testing. View results

Lendude’s picture

Status: Needs work » Needs review
FileSize
6.49 KB
13.56 KB

This might be green....

Added '(active tab)' to the tabs test to make it pass, which adds nice coverage for accessibility which I think is a nice bonus.

dawehner’s picture

+++ b/core/modules/system/tests/src/Functional/Menu/MenuRouterTest.php
index 0000000000..1ec1efee38
--- /dev/null

--- /dev/null
+++ b/core/modules/system/tests/src/Functional/Menu/MenuTestBase.php

+++ b/core/modules/system/tests/src/Functional/Menu/MenuTestBase.php
+++ b/core/modules/system/tests/src/Functional/Menu/MenuTestBase.php
@@ -0,0 +1,11 @@

@@ -0,0 +1,11 @@
+<?php
+
+namespace Drupal\Tests\system\Functional\Menu;
+
+use Drupal\Tests\BrowserTestBase;
+
+abstract class MenuTestBase extends BrowserTestBase {
+
+  use AssertBreadcrumbTrait;
+
+}

Maybe this is a little bit of a provocative question: If this is all MenuTestBase is doing, should we really have such a base class in the first place?

naveenvalecha’s picture

I think MenuTestBase is the only base class that just only has the trait. How about not converting this Base class during this conversion and directly use the BTB and use that trait in the child classes?

dawehner’s picture

I think MenuTestBase is the only base class that just only has the trait. How about not converting this Base class during this conversion and directly use the BTB and use that trait in the child classes?

Yeah this is exactly what I've been thinking as well.

naveenvalecha’s picture

Accommodated #8


#9 & #10 Its good sign that we're on same page to move this forward.

Status: Needs review » Needs work

The last submitted patch, 11: 2887099-11.patch, failed testing. View results

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.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
5.47 KB
13.48 KB

It looks like #11 did not include the fixes done in #7. Including them in this patch.

I had to manually check for these, hopefully I didn't miss anything.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Nice and minimal, everything left in /core/modules/system/src/Tests/Menu is deprecated, all tests have been moved out.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2887099-14.patch, failed testing. View results

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

UNrelated fails

  • catch committed 297c35e on 8.5.x
    Issue #2887099 by naveenvalecha, Lendude, Manuel Garcia: Menu: convert...

  • catch committed 0c90619 on 8.4.x
    Issue #2887099 by naveenvalecha, Lendude, Manuel Garcia: Menu: convert...

catch credited catch.

catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

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