Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Closed (won't fix)

Need to rewrite the whole module to make test sync with current test implementation. For more details, please refer: #1988802: [META] Rewrite test modules in system to provide better unit testing.

ayelet_Cr’s picture

Status: Closed (won't fix) » Active
mparker17’s picture

Assigned: Unassigned » mparker17

I'll help!

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Active » Closed (fixed)

This appears to be already done in commit 22df596! :D

vijaycs85’s picture

Status: Closed (fixed) » Active

The commit mentioned in #4 isolated ajax_test_dialog_contents(), but it is not removing it. Still we have a controller method that calls ajax_test_dialog_contents().

InternetDevels’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
3.49 KB

Let's see...

Pinolo’s picture

Assigned: Unassigned » Pinolo
Issue tags: +DrupalDays Milano 2014
Pinolo’s picture

Patch #6 doesn't apply any more. Attaching an equivalent patch that applies to latest HEAD.

Status: Needs review » Needs work

The last submitted patch, 8: system-ajax_test_dialog_contents-1987612-8.patch, failed testing.

Mile23’s picture

Issue tags: +Needs reroll
Mile23’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.97 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 11: 1987612_11.patch, failed testing.

Pinolo’s picture

Assigned: Pinolo » Unassigned
valthebald’s picture

FileSize
3.69 KB
valthebald’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +#SprintWeekend2015
FileSize
3.8 KB
1.51 KB

There were 2 reasons for failing tests:

  1. \Drupal\ajax_test\Controller\AjaxTestController::dialogContents() must be declared as static
  2. Removed function ajax_test_dialog_contents() was called from AjaxTestDialogForm::dialog()

Both fixed, see attached patch

David Hernández’s picture

Issue tags: -#SprintWeekend2015 +SprintWeekend2015

Hello!

Thank you for working on this issue!

We should all try and use the same sprint tag. According to https://groups.drupal.org/node/447258 it should be SprintWeekend2015 with no #.

Mile23’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
@@ -17,9 +17,26 @@ class AjaxTestController {
   /**
    * Returns example content for dialog testing.
    */
-  public function dialogContents() {
-    // Re-use the utility method that returns the example content.
-    return ajax_test_dialog_contents();
+  public static function dialogContents() {

Add @return to the docblock, please. https://www.drupal.org/coding-standards/docs#return

valthebald’s picture

Status: Needs work » Needs review
FileSize
3.91 KB

Added @return to docblock per #17

Mile23’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/tests/modules/ajax_test/src/Controller/AjaxTestController.php
@@ -15,11 +15,30 @@
   /**
-   * Returns example content for dialog testing.
+   * Example content for dialog testing.
+   * @return array
+   *   Renderable array of AJAX dialog contents.
    */
-  public function dialogContents() {

Thanks... But now there has to be an empty line before the @return annotation. :-)

So like this:

  /**
   * Example content for dialog testing.
   *
   * @return array
   *   Renderable array of AJAX dialog contents.
   */
valthebald’s picture

Status: Needs work » Needs review
FileSize
3.92 KB

Here we go!

valthebald’s picture

FileSize
3.92 KB

Removed extra space in separator comment line

The last submitted patch, 20: 1987612-20.patch, failed testing.

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

Diggit. :-)

Refactors ajax_test_dialog_contents() into AjaxTestController::dialogContents().

The last submitted patch, 14: 1987612_43.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
git ac https://www.drupal.org/files/issues/1987612-21.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  4012  100  4012    0     0  45828      0 --:--:-- --:--:-- --:--:-- 47200
error: patch failed: core/modules/system/tests/modules/ajax_test/src/Form/AjaxTestDialogForm.php:97
error: core/modules/system/tests/modules/ajax_test/src/Form/AjaxTestDialogForm.php: patch does not apply
aspilicious’s picture

Status: Needs work » Needs review
aspilicious’s picture

FileSize
4.18 KB
aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

I just rerolled this one. ANd triple checked my reroll was complete.

Wim Leers’s picture

Issue tags: -Needs reroll

Looks good!

RTBC++

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed fa9e185 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed fa9e185 on 8.0.x
    Issue #1987612 by valthebald, Mile23, aspilicious, Pinolo,...

Status: Fixed » Closed (fixed)

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