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
robeano’s picture

Assigned: Unassigned » robeano

Working on this one.

robeano’s picture

Title: Convert batch_test_nested_drupal_form_submit() to a new style controller » Convert batch_test page callbacks to a new style controller
Status: Active » Needs review
FileSize
8.69 KB

Adding a patch which converts the page callbacks in batch_test module to use the new BatchTestController.

Changes made:

  • replaced hook_menu items with routes in batch_test.routing.yml
  • moved corresponding page callback functions into BatchTestController as methods

This patch covers page callbacks. Forms have not been converted.

This issue now covers the needs of other page callback issues which will be marked as duplicates:

#1987622: Convert batch_test_large_percentage() to a new style controller
#1987626: Convert batch_test_no_form() to a new style controller
#1987628: Convert batch_test_programmatic() to a new style controller
#1987632: Convert batch_test_theme_batch() to a new style controller
#1987630: Convert batch_test_redirect_page() to a new style controller

Status: Needs review » Needs work

The last submitted patch, batch_test-1987624-4.patch, failed testing.

robeano’s picture

Status: Needs work » Needs review
FileSize
4.77 KB
8.64 KB

A couple of the controllers needed to support path arguments. The routes have been updated. Also, the controller methods were renamed to simplify things a bit. Ready for a review.

disasm’s picture

Status: Needs review » Needs work

This is almost RTBC! A couple comments below:

  1. +++ b/core/modules/system/tests/modules/batch_test/batch_test.module
    @@ -41,53 +41,6 @@ function batch_test_menu() {
    -  $items['batch-test/programmatic'] = array(
    -    'title' => 'Programmatic',
    -    'page callback' => 'batch_test_programmatic',
    -    'access callback' => TRUE,
    -    'type' => MENU_LOCAL_TASK,
    -    'weight' => 3,
    -  );
    -  // No form: fire a batch simply by accessing a page.
    -  $items['batch-test/no-form'] = array(
    -    'title' => 'Simple page',
    -    'page callback' => 'batch_test_no_form',
    -    'access callback' => TRUE,
    -    'type' => MENU_LOCAL_TASK,
    -    'weight' => 4,
    -  );
    -  // No form: fire a batch; return > 100% complete
    -  $items['batch-test/large-percentage'] = array(
    -    'title' => 'Simple page with batch over 100% complete',
    -    'page callback' => 'batch_test_large_percentage',
    -    'access callback' => TRUE,
    -    'type' => MENU_LOCAL_TASK,
    -    'weight' => 5,
    -  );
    -  // Tests programmatic form submission within a batch operation.
    -  $items['batch-test/nested-programmatic'] = array(
    -    'title' => 'Nested programmatic',
    -    'page callback' => 'batch_test_nested_drupal_form_submit',
    -    'access callback' => TRUE,
    -    'type' => MENU_LOCAL_TASK,
    -    'weight' => 6,
    -  );
    -  // Landing page to test redirects.
    -  $items['batch-test/redirect'] = array(
    -    'title' => 'Redirect',
    -    'page callback' => 'batch_test_redirect_page',
    -    'access callback' => TRUE,
    -    'type' => MENU_LOCAL_TASK,
    -    'weight' => 7,
    

    Because these aren't MENU_CALLBACK, the menu items should remain, remove page callback and access callback, and add 'route_name' => 'name_of_route_in_yaml_file'.

  2. +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Controller/BatchTestController.php
    @@ -0,0 +1,101 @@
    +  }
    +}
    

    need a blank line between last two closing braces. Also missing a newline at end of file.

piyuesh23’s picture

Assigned: robeano » piyuesh23
FileSize
2.45 KB
8.75 KB

Updated the Patch with the information mentioned in the previous comment. Uploading the new patch and interdiff file.

piyuesh23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, batch_test-1987624-8.patch, failed testing.

disasm’s picture

Thanks for picking this up piyuesh23. Note that we aren't doing any major cleanups for test modules (creating services, moving helper methods into controllers, etc... We just need to get tests working again. That being said, there are some minor cleanups that shouldn't take much more than 5 minutes to do. I've listed them below.

  1. +++ b/core/modules/system/tests/modules/batch_test/batch_test.routing.yml
    @@ -0,0 +1,41 @@
    +  pattern: 'batch-test/redirect'
    ...
    +  pattern: 'batch-test/large-percentage'
    ...
    +  pattern: 'batch-test/nested-programmatic/{value}'
    ...
    +  pattern: 'batch-test/no-form'
    ...
    +  pattern: 'batch-test/programmatic/{value}'
    ...
    +  pattern: 'admin/batch-test/test-theme'
    

    patterns should begin with a leading '/'

  2. +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Controller/BatchTestController.php
    @@ -0,0 +1,102 @@
    +use Drupal\Core\Controller\ControllerInterface;
    ...
    +class BatchTestController implements ControllerInterface {
    

    Since we're not injecting anything, lets just make this a plain class and not extend/implement anything.

  3. +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Controller/BatchTestController.php
    @@ -0,0 +1,102 @@
    +use Symfony\Component\DependencyInjection\ContainerInterface;
    ...
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(ContainerInterface $container) {
    +    return new static();
    +  }
    +
    

    not needed. No services are injected in this controller.

  4. +++ b/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Controller/BatchTestController.php
    @@ -0,0 +1,102 @@
    +   * Menu callback: Redirects successfuly.
    ...
    +   * Menu callback: Submits a form within a batch programmatically.
    ...
    +   * Menu callback: Fires a batch process without a form submission.
    ...
    +   * Menu callback: Submits the 'Chained' form programmatically.
    ...
    +   * Menu callback: Runs a batch for testing theme used on the progress page.
    

    We don't call controller methods menu callbacks. start with the description, no need for the prefix.

piyuesh23’s picture

@disasm Thanks for reviewing and helping out with this issue. Uploading the fixed patch and interdiff files.

piyuesh23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, batch_test-1987624-12.patch, failed testing.

disasm’s picture

+++ b/core/modules/system/tests/modules/batch_test/batch_test.module
@@ -45,47 +45,41 @@ function batch_test_menu() {
     'title' => 'Programmatic',
...
     'title' => 'Simple page',
...
     'title' => 'Simple page with batch over 100% complete',
...
     'title' => 'Nested programmatic',
...
     'title' => 'Redirect',

Lets use _title property in the routing.yml file.

disasm’s picture

Status: Needs work » Needs review
FileSize
3.19 KB
8.68 KB

Spent some time debugging these test failures, and these are passing locally now. Here's the patch. We needed a default set for value. Also, fixed the titles above, so this should be green and ready to be reviewed. Thanks piyuesh for all your work on this!

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

The last submitted patch, drupal8.system-module.1987624-16.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion
dawehner’s picture

  1. +++ w/core/modules/system/tests/modules/batch_test/batch_test.module
    @@ -44,48 +44,37 @@ function batch_test_menu() {
       // This item lives under 'admin' so that the page uses the admin theme.
       $items['admin/batch-test/test-theme'] = array(
    -    'page callback' => 'batch_test_theme_batch',
    -    'access callback' => TRUE,
    +    'route_name' => 'batch_test_test_theme',
         'type' => MENU_CALLBACK,
       );
     
    

    That one is really just a menu callback, so can be dropped.

  2. +++ w/core/modules/system/tests/modules/batch_test/lib/Drupal/batch_test/Controller/BatchTestController.php
    @@ -0,0 +1,93 @@
    +  /**
    +   * Redirects successfuly.
    +   */
    +  public function testRedirect() {
    ...
    +  /**
    +   * Fires a batch process without a form submission.
    +   */
    ...
    +  /**
    +   * Submits a form within a batch programmatically.
    +   */
    +  public function testNestedDrupalFormSubmit($value = 1) {
    ...
    +  /**
    +   * Fires a batch process without a form submission.
    +   */
    ...
    +  /**
    +   * Submits the 'Chained' form programmatically.
    +   *
    +   * Programmatic form: the page submits the 'Chained' form through
    +   * drupal_form_submit().
    +   */
    ...
    +  /**
    +   * Runs a batch for testing theme used on the progress page.
    +   */
    

    I guess we should put some @return and some @param on there, just short ones.

xjm’s picture

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

dawehner’s picture

Status: Needs review » Needs work

.

disasm’s picture

Status: Needs work » Needs review
FileSize
3.1 KB
152.35 KB

addresses comments in #19. $value param has no description, because digging into the functions that it's passed to explains about as much as these methods do.

dawehner’s picture

Status: Needs review » Needs work

This diff seems too big.

disasm’s picture

Status: Needs work » Needs review
FileSize
9.48 KB

try again. no interdiff, same as above.

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

The last submitted patch, drupal8.batch_test.1987624-24.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion
disasm’s picture

I ran a few simple tests as well as enabled content translation for pages. Are there any specific other manual tests that need done? I experienced no errors in my manual testing.

disasm’s picture

#27 was meant for a different issue.

dawehner’s picture

FileSize
1.02 KB
9.58 KB

Just fixing some tiny docs

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Patch is green and we already have tests for this code so i think it is good to go.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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