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

Assigned: Unassigned » kgoel

Going to work on this.

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.

brianV’s picture

Status: Active » Needs review
FileSize
0 bytes

First attempt at a patch. Thanks @larowlan for walking me through it so far.

kgoel’s picture

Assigned: kgoel » Unassigned
brianV’s picture

Assigned: Unassigned » kgoel
FileSize
5.13 KB

And... this time without an empty patch file. Sorry all!

brianV’s picture

Assigned: kgoel » brianV

cross posted

kgoel’s picture

Assigned: brianV » Unassigned
kgoel’s picture

I think we made edit at the same time.

brianV’s picture

Assigned: Unassigned » brianV

Last cross post of the night ;)

larowlan’s picture

Assigned: brianV » Unassigned

Great job, talk about jumping in at the deep end!

  1. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Controller/FormTestController.php
    @@ -0,0 +1,42 @@
    +use Drupal\Core\Controller\ControllerBase;
    

    chuck a blank line between the namespace and use statements as per http://drupal.org/node/1354

  2. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Controller/FormTestController.php
    @@ -0,0 +1,42 @@
    +  /**
    +   * Constructs a FormTestController object.
    +   */
    +  public function __construct() {
    +  }
    

    This empty method can go thanks to ControllerBase having all the stuff we need.

  3. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Controller/FormTestController.php
    @@ -0,0 +1,42 @@
    +   *
    

    Extra blank line can go

  4. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Controller/FormTestController.php
    @@ -0,0 +1,42 @@
    +  function twoFormInstances() {
    

    missing 'public'

  5. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Controller/FormTestController.php
    @@ -0,0 +1,42 @@
    +}
    
    +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/EventSubscriber/FormTestEventSubscriber.php
    @@ -33,8 +52,25 @@ public function onKernelRequest(GetResponseEvent $event) {
     }
    

    Needs a blank line before last }

brianV’s picture

See attached, should resolve the above.

larowlan’s picture

  1. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Controller/FormTestController.php
    @@ -0,0 +1,38 @@
    +  ¶
    
    +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/EventSubscriber/FormTestEventSubscriber.php
    @@ -33,8 +52,26 @@ public function onKernelRequest(GetResponseEvent $event) {
    +  ¶
    

    nitpick:trailing whitespace

  2. +++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/EventSubscriber/FormTestEventSubscriber.php
    @@ -17,6 +21,21 @@
    +
    +
    

    nitpick:can we loose one of these extra lines

Other than that RTBC in my opinion, assuming bot agrees

brianV’s picture

What's a nitpick between friends?

Hopefully final revision attached.

larowlan’s picture

+++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/Controller/FormTestController.php
@@ -0,0 +1,38 @@
\ No newline at end of file

+++ b/core/modules/system/tests/modules/form_test/lib/Drupal/form_test/EventSubscriber/FormTestEventSubscriber.php
@@ -33,8 +51,26 @@ public function onKernelRequest(GetResponseEvent $event) {
\ No newline at end of file

sorry I've confused you :( there were lines with whitespace, but the ones at the end of the file had to stay

brianV’s picture

Good. I thought you had suggested otherwise ;)

re-roll!

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Unless bot disagrees.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal8.system-module.1987718-17.patch, failed testing.

brianV’s picture

Status: Needs work » Needs review
FileSize
5.79 KB

New version with the two changes:

1. I never removed the original form_test_two_instances() callback. That's done now.

2. EntityManager::getStorageController('node')->create(); doesn't seem to create the node form with the form display settings set by entity_get_form_display()->setComponent()->save() into account. Therefore, the test failed in the above patch because the AJAX field created specifically for that test wasn't being added to the node form.

I asked about #2 in #drupal-contribute a few times, but got no responses, so I've reverted back to entity_create() in the controller to see if that fixes it.

Status: Needs review » Needs work

The last submitted patch, drupal8.system-module.1987718-20.patch, failed testing.

brianV’s picture

This should work, passes all tests locally.

Turns out the issue was this the permissions for the router item that prevented the test user from seeing the page.

Reverted back to EntityManager::getStorageController('node')->create(); to create the node objects for the node forms.

brianV’s picture

Status: Needs work » Needs review

bumping back to NR

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Legend, thanks for coming back and working through it

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great work! I found a couple of small indentation problems on the comments, but fixed those on commit.

Committed and pushed to 8.x. Thanks!

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