CommentFileSizeAuthor
#20 node-admin-2389333-test-coverage-20.patch4.33 KBswaps
#17 node-admin-2389333-test-coverage-17.patch4.3 KBsaurabh-chugh
#14 node-admin-2389333-test-coverage-14.patch4.28 KBswaps
#9 node-admin-2389333-diff-7-9.txt949 bytesvijaycs85
#9 node-admin-2389333-test-coverage-9.patch5.09 KBvijaycs85
#7 node-admin-2389333-diff-2-7.txt4.37 KBvijaycs85
#7 node-admin-2389333-test-coverage-7.patch5.09 KBvijaycs85
#2 node-admin-2389333-test-coverage-2.patch5.04 KBvijaycs85

Comments

pacproduct’s picture

pacproduct commented
Title: Let's add test coverate » Let's add test coverage
vijaycs85’s picture

vijaycs85
Location London, UK
commented
Status: Active » Needs review
StatusFileSize
new node-admin-2389333-test-coverage-2.patch5.04 KB

Initial patch...

Status: Needs review » Needs work

The last submitted patch, 2: node-admin-2389333-test-coverage-2.patch, failed testing.

vijaycs85’s picture

vijaycs85
Location London, UK
commented
Version: 7.x-1.x-dev » 6.x-1.x-dev

Wrong version...

Status: Needs work » Needs review

vijaycs85 queued 2: node-admin-2389333-test-coverage-2.patch for re-testing.

pacproduct’s picture

pacproduct commented

My review comments below.

  1. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+   * Set up some required modules

    (minor) Period missing. 

  2. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+    // We can only test the plugins that don't require other modules

    (minor) Period missing. 

  3. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+

    (minor) Unnecessary blank line. 

  4. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+    // or invalid content type.

    (minor) Capital letter for "or" maybe? 

  5. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+    // Create a new page node

    (minor) Period missing. 

  6. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+

    I would have put the blank line after $this->drupalLogin() instead.
    Or none at all here. 

  7. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+    // Create a page node and a story

    (minor) Period missing. 

  8. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+    // Only specific content type in it's own admin page.

    (minor) Should be "its". 

  9. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+   *   Node object to asset.

    Typo: Should be "assert". 

  10. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+   *

    Is that empty line needed (applies to other functions as well)? 

  11. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+

    (minor) Unnecessary blank line. 

  12. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+   *   Node object to asset.

    Typo: Should be "assert". 

  13. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+    // Make sure node present with link.

    Comment doesn't match what is being tested. 

  14. +++ b/tests/node_admin_ui.test
@@ -0,0 +1,143 @@
+

    (minor) Unnecessary blank line.

vijaycs85’s picture

vijaycs85
Location London, UK
commented
StatusFileSize
new node-admin-2389333-test-coverage-7.patch5.09 KB
new node-admin-2389333-diff-2-7.txt4.37 KB

thanks for the review @pacproduct. Here is an update for all items in #6

pacproduct’s picture

pacproduct commented

Looks okay to me.

vijaycs85’s picture

vijaycs85
Location London, UK
commented
StatusFileSize
new node-admin-2389333-test-coverage-9.patch5.09 KB
new node-admin-2389333-diff-7-9.txt949 bytes

Few more empty line fixes.

pacproduct’s picture

pacproduct commented
Status: Needs review » Reviewed & tested by the community

  • vijaycs85 committed 11933bc on 6.x-1.x 
    Issue #2389333 by vijaycs85, pacproduct: Lets add test coverage
vijaycs85’s picture

vijaycs85
Location London, UK
commented
Status: Reviewed & tested by the community » Fixed
vijaycs85’s picture

vijaycs85
Location London, UK
commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Fixed » Active

Let's port to D7

swaps’s picture

swaps commented
StatusFileSize
new node-admin-2389333-test-coverage-14.patch4.28 KB

Machi ,

Attached is the patch for D7 port

Cheers
SwapS

swaps’s picture

swaps commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: node-admin-2389333-test-coverage-14.patch, failed testing.

saurabh-chugh’s picture

saurabh-chugh commented
StatusFileSize
new node-admin-2389333-test-coverage-17.patch4.3 KB

Hey

Looks like one of the required permission is missing.
Re-attaching updated patch

Saurabh Chugh

saurabh-chugh’s picture

saurabh-chugh commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: node-admin-2389333-test-coverage-17.patch, failed testing.

swaps’s picture

swaps commented
StatusFileSize
new node-admin-2389333-test-coverage-20.patch4.33 KB

One more update ..
This should pass test run

Cheers
SwapS

saurabh-chugh’s picture

saurabh-chugh commented
Status: Needs work » Needs review

  • vijaycs85 committed 0aa1dd6 on 7.x-1.x authored by SwapS 
    Issue #2389333 by vijaycs85, SwapS, saurabh-chugh: Let's add test...
vijaycs85’s picture

vijaycs85
Location London, UK
commented
Status: Needs review » Reviewed & tested by the community

Thank you!

vijaycs85’s picture

vijaycs85
Location London, UK
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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