Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plopesc’s picture

Status: Active » Needs review
FileSize
3.93 KB

Hello
Attaching patch that convert tour_test_1() and tour_test_2() #1979010: Convert tour_test_2() to a Controller to a Controller.
Regards

dawehner’s picture

This looks basically perfect.

+++ b/core/modules/tour/tests/tour_test/lib/Drupal/tour_test/Controller/TourTestController.phpundefined
@@ -0,0 +1,66 @@
+  /**
+   * Constructs a TourTestController object.
+   */
+  public function __construct() {
+  }

If we don't inject anything there is no need for a constructor.

plopesc’s picture

Re-rolling patch

Status: Needs review » Needs work

The last submitted patch, tour_test_controller-1979004-3.patch, failed testing.

plopesc’s picture

Status: Needs work » Needs review
FileSize
3.83 KB

Re-rolling patch excluding .swp files

Sorry

dawehner’s picture

+++ b/core/modules/tour/tests/tour_test/lib/Drupal/tour_test/Controller/TourTestController.phpundefined
@@ -0,0 +1,60 @@
+* Controller routines for tour_test routes.
+*/

needs indentation, sorry I should have spotted this the first time.

plopesc’s picture

No worries
Re-rolled.

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, tour_test_controller-1979004-7.patch, failed testing.

plopesc’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, tour_test_controller-1979004-7.patch, failed testing.

plopesc’s picture

Status: Needs work » Needs review

I can't figure why tests are failing now. Diff between patch in #5 and #7 are only comment indentations.

Moreover, test fail comes from an unrelated project, I think.

Maybe the failing test is related to one of the last commits.

Regards.

dawehner’s picture

plopesc’s picture

After problems with test bot, this patch is green :)

@dawehner: Could be marked as RTBC now?

Regards

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 6df1d3a and pushed to 8.x. Thanks!

tim.plunkett’s picture

Status: Fixed » Needs review
FileSize
1.03 KB
+++ b/core/modules/tour/tests/tour_test/lib/Drupal/tour_test/Controller/TourTestController.phpundefined
@@ -0,0 +1,60 @@
+use Drupal\Core\ControllerInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
...
+class TourTestController implements ControllerInterface {
...
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();

This is unnecessary and vaguely confusing. We should avoid doing this in the future.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

agreed

ParisLiakos’s picture

oops

+++ b/core/modules/tour/tests/tour_test/lib/Drupal/tour_test/Controller/TourTestController.phpundefined
@@ -6,20 +7,10 @@
+ * Retursn responses for tour_test routes.

returns?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Let's fix the typo...

ParisLiakos’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.03 KB

sure quickly manually edited the patch

Status: Reviewed & tested by the community » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, tour-1979004-20.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion

#20: tour-1979004-20.patch queued for re-testing.

ParisLiakos’s picture

Title: Convert tour_test_1() to a Controller » Followup: Convert tour_test_1() to a Controller
Status: Needs review » Reviewed & tested by the community

it was a random failure:P
i think i can rtbc it, if i just have fixed a typo right?:P

tim.plunkett’s picture

Title: Followup: Convert tour_test_1() to a Controller » Convert tour_test_1() to a Controller
Status: Reviewed & tested by the community » Fixed

After some talk with @msonnabaum and @Crell, we're just going to let these happen. It's of course rather silly for a test controller, but it is useful for real ones in case something ever needs to be injected.

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

Anonymous’s picture

Issue summary: View changes

Added link to META task