Problem/Motivation

#2073811-154: Add a url generator twig extension got me thinking that we're missing test coverage and sure enough we have no test coverage of visiting node/add with no node types configured.

Proposed resolution

Add missing test coverage toDrupal\node\Tests\NodeCreationTest - note similar to testNodeCreation you will need to delete the content types created during setUp at the beginning of the test.

Remaining tasks

 

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Y Instructions Y
Update the issue summary Y Instructions Y
Add automated tests Y Instructions Y

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

Issue summary: View changes
mitsuroseba’s picture

Assigned: Unassigned » mitsuroseba
anavarre’s picture

Trying to understand what's the scope for this issue: should the test live in a new function or would adding it in the testNodeCreation() function be acceptable?

Also, I did try to add the below test (temporarily) in the testNodeCreation() function but then it throws a Simpletest failure as a 403 is returned while a 200 is expected:

entity_load('node_type', 'page')->delete();
$this->drupalGet('node/add');
$this->assertResponse(200);

I've noticed that the only way to fix it is to add the 'administer content types' permission to the user object.

P.S.: I did try that with and without Cottser's patch in #2073811: Add a url generator twig extension

alexpott’s picture

A full test of the node/add functionality would include testing for users with and without 'administer content types'. For users with the 'administer content types' permission we should be using WebTest's click link functionality to test that the expected link is there.

alexpott’s picture

Cottser's patch in #2073811: Add a url generator twig extension is fine and works it's @dawehner's patch that exposed the missing test coverage.

mitsuroseba’s picture

Assigned: mitsuroseba » Unassigned
Status: Active » Needs review
FileSize
1.22 KB
alexpott’s picture

Status: Needs review » Needs work

Nice work - a few minor things to keep the test as simple and reliable as possible.

  1. +++ b/core/modules/node/src/Tests/NodeCreationTest.php
    @@ -158,4 +158,30 @@ public function testAuthorAutocomplete() {
    +  function testNodeCreationWithoutContentTypes() {
    

    Lets call this testNodeAddWithoutContentTypes since you're not creating a node.

  2. +++ b/core/modules/node/src/Tests/NodeCreationTest.php
    @@ -158,4 +158,30 @@ public function testAuthorAutocomplete() {
    +    // Test /node/add page without content types.
    +    foreach (entity_load_multiple('node_type') as $entity ) {
    +      $entity->delete();
    +    }
    +
    +    $this->drupalGet('node/add');
    +    $this->assertResponse(403);
    

    I would assert that the current user can get to node/add before deleting the content types - in it's current state this seems a bit brittle.

  3. +++ b/core/modules/node/src/Tests/NodeCreationTest.php
    @@ -158,4 +158,30 @@ public function testAuthorAutocomplete() {
    +    $this->assertUrl('node/add');
    ...
    +    $this->assertUrl('node/add');
    

    You only really need to do this is you are expecting the url to be different as is the case in testNodeCreation

  4. +++ b/core/modules/node/src/Tests/NodeCreationTest.php
    @@ -158,4 +158,30 @@ public function testAuthorAutocomplete() {
    +    $this->drupalGet('admin/structure/types/add');
    +    $this->assertResponse(200);
    +    $this->assertUrl('/admin/structure/types/add');
    

    We don't need to actually go to the node type add page - this is tested elsewhere.

mitsuroseba’s picture

Thank you for review. Here a new patch.

mitsuroseba’s picture

Status: Needs work » Needs review
alexpott’s picture

Status: Needs review » Needs work

Nearly perfect! Just a couple of very very minor nitpicks.

+++ b/core/modules/node/src/Tests/NodeCreationTest.php
@@ -158,4 +158,27 @@ public function testAuthorAutocomplete() {
+    $this->drupalGet('node/add');
+    $this->assertResponse(200);
+
+    $this->assertLinkByHref('/admin/structure/types/add', 0);

Actually there is no need to assert the 200 here if you are going to assert the link. And the assertLinkByHref's second argument is not needed. The default is 0.

mitsuroseba’s picture

Status: Needs work » Needs review
FileSize
1.05 KB
443 bytes
alexpott’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Great! Thank you.

dawehner’s picture

+++ b/core/modules/node/src/Tests/NodeCreationTest.php
@@ -158,4 +158,26 @@ public function testAuthorAutocomplete() {
+    $this->drupalGet('node/add');
+    $this->assertResponse(200);
+
+    // Test /node/add page without content types.
+    foreach (entity_load_multiple('node_type') as $entity ) {
+      $entity->delete();
+    }

Can we have a assertNoLinkByHref in case we do have node types?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Good point @dawehner

mitsuroseba’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
534 bytes
alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

dawehner’s picture

perfect

catch’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.0.x, thanks!

Could probably be backported.

  • catch committed 6749237 on 8.0.x
    Issue #2309673 by mitsuroseba: Add tests for node/add when no node types...
pbull’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.14 KB

Here's a D7 backport that adds this test to NodeCreationTestCase.

Status: Needs review » Needs work

The last submitted patch, 21: d7-test-no-node-type-exist-2309673-21.patch, failed testing.

pbull’s picture

Status: Needs work » Needs review
FileSize
1.19 KB
444 bytes

Trying again... previous D7 patch passes locally but this adds node and menu rebuilds.

Status: Needs review » Needs work

The last submitted patch, 23: d7-test-no-node-type-exist-2309673-23.patch, failed testing.

m1r1k’s picture

Issue tags: +#ams2014contest

  • catch committed 6749237 on 8.1.x
    Issue #2309673 by mitsuroseba: Add tests for node/add when no node types...
mitsuroseba’s picture

Status: Needs work » Needs review
Issue tags: -

Retest

  • catch committed 6749237 on 8.3.x
    Issue #2309673 by mitsuroseba: Add tests for node/add when no node types...

  • catch committed 6749237 on 8.3.x
    Issue #2309673 by mitsuroseba: Add tests for node/add when no node types...

  • catch committed 6749237 on 8.4.x
    Issue #2309673 by mitsuroseba: Add tests for node/add when no node types...

  • catch committed 6749237 on 8.4.x
    Issue #2309673 by mitsuroseba: Add tests for node/add when no node types...