This is simply a forward port of the docblock from #1302228: field_has_data() returns inconsistent results – possible data loss.

Files: 
CommentFileSizeAuthor
#62 node_test_docblock-1540094-62.patch3.11 KBbdgreen
PASSED: [[SimpleTest]]: [MySQL] 40,525 pass(es).
[ View ]
#58 node_test_docblock-1540094-58.patch3.61 KBbdgreen
PASSED: [[SimpleTest]]: [MySQL] 40,331 pass(es).
[ View ]
#56 node_test_docblock-1540094-56.patch3.45 KBbdgreen
PASSED: [[SimpleTest]]: [MySQL] 40,080 pass(es).
[ View ]
#54 node_test_docblock-1540094-54.patch2.47 KBbdgreen
PASSED: [[SimpleTest]]: [MySQL] 40,491 pass(es).
[ View ]
#52 node_test_docblock-1540094-52.patch16.52 KBbdgreen
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#50 node_test_docblock-1540094-50.patch10.17 KBbdgreen
PASSED: [[SimpleTest]]: [MySQL] 40,487 pass(es).
[ View ]
#47 node_test_docblock-1540094-47.patch1.14 KBbdgreen
PASSED: [[SimpleTest]]: [MySQL] 57,167 pass(es).
[ View ]
#35 node-test-docblock-1540094-35.patch1.33 KBrootwork
PASSED: [[SimpleTest]]: [MySQL] 53,276 pass(es).
[ View ]
#31 node-test-docblock-1540094-31.patch1.33 KBrootwork
PASSED: [[SimpleTest]]: [MySQL] 52,586 pass(es).
[ View ]
#27 node-test-docblock-1540094-27.patch1.2 KBdermario
PASSED: [[SimpleTest]]: [MySQL] 52,212 pass(es).
[ View ]
#20 node-test-docblock-1540094-20.patch27.76 KBkilltheliterate
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-test-docblock-1540094-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 node-test-docblock-1540094-15.patch19.31 KBpesgyo
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-test-docblock-1540094-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 node-test-docblock-3.patch15.93 KBNoe_
PASSED: [[SimpleTest]]: [MySQL] 40,256 pass(es).
[ View ]
#7 node-test-docblock-2.patch58.07 KBNoe_
PASSED: [[SimpleTest]]: [MySQL] 40,118 pass(es).
[ View ]
node-test-docblock.patch424 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 35,356 pass(es).
[ View ]

Comments

Status:Active» Needs review

Issue tags:+Novice

Good patch for a novice to review. :)

Status:Needs review» Needs work

This is (or at least should be) part of the API Cleanup for Node issue right?

Oh, I see that one is already on to 7.x. What happened here -- why is this patch needed? Did we miss it or was it added recently?

Also, I see that node.test has several other classes without doc blocks. Perhaps we should reopen the other issue?
#1313980: Clean up API docs for node module
for Drupal 8?

Well, this class was added as part of a patch committed long after the node.module cleanup was committed to D8:
#1373142: Use the Testing profile. Speed up testbot by 50%
So unrelated to any other classes that were missed in the cleanup.

Edit: version correction.

Title:Add a docblock for NodeWebTestCaseAdd missing docblocks for classes and methods in node.test
Issue tags:+needs backport to D7

Alright, @jhodgdon and I discussed this in IRC, and we agreed to expand this issue to cover the other missing docblocks in the file. Anyone that likes is welcome to work on this. :)

There are currently 37 test classes in D8's node module, and most have docblocks. To get a specific listing of files/classes/methods that need them, see:

http://drupal.org/node/1359786#comment-6286462

StatusFileSize
new58.07 KB
PASSED: [[SimpleTest]]: [MySQL] 40,118 pass(es).
[ View ]

Attached is a patch for review.

Status:Needs work» Needs review

Thanks @Noe_! Remember to set the status to "Needs Review" when you update the patch. :)

Status:Needs review» Needs work

This patch has non-documentation changes in it, such as:

-  function setUp() {
+  public function setUp() {

Please remove those, and stick to the issue we are dealing with, thanks!

Status:Needs work» Needs review
StatusFileSize
new15.93 KB
PASSED: [[SimpleTest]]: [MySQL] 40,256 pass(es).
[ View ]

Removed all the code improvements, and I just stuck to documentation issues.

Except setUp() and getInfo() functions because @xjm told me not to document those.

Status:Needs review» Needs work

Thanks for the new patch! It's a great start. A few things to address:

a)

+  /**
+   * Test the "rebuild permissions" link.
+   */
   function testNodeAccessRebuild() {

Wrong verb tense -- see http://drupal.org/node/1354#functions -- this applies to the other doc blocks in the patch too.

b)

-    // Create and login user
+    // Create and login user.

If you are going to fix this line, you could also change "login" (an adjective) to "log in" (a verb).

c)

+   * Test to ensure that a node's content array is rebuilt on every
+   * call to node_build_content().

Wrapping - try to put as much on the first line as possible without going over 80 characters.

d)

+   * Test if custom data is added to the RSS feed.

Grammar: Change "test if" to "test whether" throughout the patch.

e)

@@ -38,6 +39,7 @@ class NodeSaveTest extends NodeTestBase {
   /**
    * Import test, to check if custom node ids are saved properly.
+   *

This needs to start with a verb.

f)

/**
+   * Tests determing changes in hool_node_presave().
+   *
    * Tests determing changes in hook_node_presave() and verifies the static node
    * load cache is cleared upon save.
    */

The added lines don't make much sense to me, and "determing" is misspelled. Also I don't think we need that text repeated twice in the documentation.

d)
+ * Test if custom data is added to the RSS feed.

I suppose this needs to be:
Tests whether, instead of test whether.

f)
Also I don't think we need that text repeated twice in the documentation.

I did that because the first line of documentation need to be limited to 80 characters. However I thought I couldn't just leave the rest out, so I copied it again but this time with the full line.
If you have a better way of doing this please let me know.

Yes, "Tests whether..." is correct.

Regarding your second question about repeated text, I would edit the second paragraph so that it doesn't contain the same information as the first line. Maybe start with "Also"?

Assigned:Unassigned» pesgyo

I'm working on it today.

Status:Needs work» Needs review
StatusFileSize
new19.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-test-docblock-1540094-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I tried to correct the errors posted by @jhodgdon and I made some other changes. This is my first patch, I'm sorry if I made some mistake.

Status:Needs review» Needs work

The last submitted patch, node-test-docblock-1540094-15.patch, failed testing.

Assigned:pesgyo» Unassigned

Status:Needs work» Needs review
Issue tags:-Novice, -needs backport to D7

#15: node-test-docblock-1540094-15.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Novice, +needs backport to D7

The last submitted patch, node-test-docblock-1540094-15.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new27.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch node-test-docblock-1540094-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled. Patch #15 assumes patch #10 has been applied already. Applies cleanly at ea1b955.

Status:Needs review» Needs work

Let's leave the in-code // comments out of this issue. The fixes are a bit difficult to review, and not all of them are correct. For instance:

-    // nodes keyed by uid and nid: $nodes[$uid][$nid] = $is_private;
+    // Nodes keyed by uid and nid: $nodes[$uid][$nid] = $is_private;

comments should end in . not ; so this is only a partial fix of this comment.

Reviewing the added/corrected documentation blocks, it looks like the comments in #11 were not addressed.

Some more comments, which might duplicate #11:
a) I found two that still have the wrong verb tense:

+  /**
    * Test multilingual field display settings.
    */
   function testMultilingualDisplaySettings() {
...
   /**
-   * Set "Basic page" content type to not display post information and confirm its absence on a new node.
+   * Test 'not display' post information.

b) And this one doesn't even start with a verb:

/**
-   * Import test, to check if custom node ids are saved properly.
+   * Import test, to check whether custom node ids are saved properly.
+   *

c) This line has a spelling error:

/**
+   * Tests determing changes in hool_node_presave().

I stopped at this point because I realize #11 had not been addressed.

Assigned:Unassigned» dermario

I will work on this on sunday, as part of Badcamp switzerland. There are mentors that may teach me how to perform automated codestyle checks.

Status:Needs work» Needs review
Issue tags:-Novice, -needs backport to D7

#20: node-test-docblock-1540094-20.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Novice, +needs backport to D7

The last submitted patch, node-test-docblock-1540094-20.patch, failed testing.

I am not sure what to do now. It seems that most of the comments are fixed in D8. Is it just the backport to D7 we need here?

Hm. Maybe the first thing to do would be to review the latest patch that was attached before, and the comments that came after, and see if anything was missed -- if so, make a Drupal 8 patch.

And if not, check Drupal 7 to see how that is with regards to those same fixes. If it's fine, mark this issue as fixed. If not, change the version to Drupal 7 and make a patch.

This sometimes happens with documentation cleanup -- sometimes it gets fixed on a different issue. Thanks!

Status:Needs work» Needs review
StatusFileSize
new1.2 KB
PASSED: [[SimpleTest]]: [MySQL] 52,212 pass(es).
[ View ]

I checked the D8 node module tests for "determing" and found 1 more occurance. Afterwards i searched for " * Test " and corrected one occurance where the comment started with a Noun.

Status:Needs review» Needs work
Issue tags:-Novice, -needs backport to D7

The last submitted patch, node-test-docblock-1540094-27.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Novice, +needs backport to D7

#27: node-test-docblock-1540094-27.patch queued for re-testing.

Status:Needs review» Needs work

The NodeSaveTest::testDeterminingChanges() documentation block should start with a single-line summary. If additional information is needed, it should be after a paragraph break.

Status:Needs work» Needs review
StatusFileSize
new1.33 KB
PASSED: [[SimpleTest]]: [MySQL] 52,586 pass(es).
[ View ]

I've updated the patch from #27 with a new single-line summary and longer paragraph for NodeSaveTest::testDeterminingChanges().

Issue tags:+#SprintWeekend

Meant to tag this as work done during the global sprint.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeSaveTest.phpundefined
@@ -128,8 +128,10 @@ function testTimestamps() {
+   * ¶

Trailing whitespace. :)

Looks like a straightforward cleanup otherwise!

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.33 KB
PASSED: [[SimpleTest]]: [MySQL] 53,276 pass(es).
[ View ]

Removed the trailing space.

Assigned:dermario» jhodgdon
Status:Needs review» Reviewed & tested by the community

Looks good, I'll get it committed soon. Thanks!

There's another issue marked "avoid commit conflicts" that also touches one of these files, so I'll wait to commit this until #1380720: Rename entity "bundle" to "subtype" is done.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, node-test-docblock-1540094-35.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Novice, +needs backport to D7, +#SprintWeekend

#35: node-test-docblock-1540094-35.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Needs work

The small patch in #35 has been committed to Drupal 8.x.

I think that there are still issues identified in previous patches (the current one started in #27) and comments that have not been patched/committed yet. So let's leave this issue at Drupal 8.x until someone can verify that the patches/comments above have been addressed.

Reviewed:

#11
a) OK
b) Not changed, so still reads "login" - OK?
c) OK
d) OK
e) Changed, but reads "custom block IDs are saved" (rather than "ids"?) - OK?
f) OK

#21
pre: Still reads as "// nodes keyed by uid and nid: $nodes[$uid][$nid] = $is_private;" Observe, still terminates with ";" rather than "."
a) OK
b) Now reads "* Checks whether custom node IDs are saved ..." (rather than "ids"?) - OK?
c) OK

#21 OK
#31 OK
#33 OK
#35 OK

Other than a patch for #21 pre: (terminate comment with "." rather than ";") should other points above be included?

Thanks a lot for the review!!!

Let's fix #11b, #11e if it doesn't start with a verb.
On #21 the code comments - out of scope for this issue.

So we need one more small patch. Any takers?

Status:Needs work» Needs review
StatusFileSize
new1.14 KB
PASSED: [[SimpleTest]]: [MySQL] 57,167 pass(es).
[ View ]

One small patch ... ;)

Version:8.x-dev» 7.x-dev

Oh, sorry about the confusion -- I think I misunderstood your analysis. "IDs" is correct -- that means "identifiers". "ids" is not correct. That is the plural of "id", which is a psychological term (ego, superego, id). The only things we should be fixing that are left over from previous patches would be first-line descriptions that do not start with verbs. In-code comments are out of scope.

So... we don't want to do either of the changes in this patch, and if everything else is fine, let's move on to 7.x, where we need a backport of all previous patches (or it might be easier to start over).

Status:Needs review» Patch (to be ported)

sorry, forgot status

Status:Patch (to be ported)» Needs review
StatusFileSize
new10.17 KB
PASSED: [[SimpleTest]]: [MySQL] 40,487 pass(es).
[ View ]

Backport to 7.x for docblocks (that is all "in-line code comments" have been ignored) in node.test. ;)

Status:Needs review» Needs work

This block shifts verb agreement/mood/whatever in the middle (from "create" to "verifies"):

+   * Create a page node and verifies that a transaction rolls back when there is
+   * a failed creation.
    */
   function testFailedPageCreation() {

There are several more that do that, such as:

+   * Creates an unpublished node and confirm correct redirect behavior.
    */
   function testUnpublishedNodeCreation() {

and the next one too... I've stopped reading the patch at this point. Can you check all of the changes in the patch for that and submit a new patch please?

Hopefully 8.x does not have this problem... If it does we should go back to 8 and fix it there.

Status:Needs work» Needs review
StatusFileSize
new16.52 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Redo of backport to 7.x for docblocks ...

Status:Needs review» Needs work

Thanks for the new patch!

There are still issues with the patch:

a)

/**
- * Defines a base class for testing the Node module.
+ * Creates Basic page and Article node types.
  */
class NodeWebTestCase extends DrupalWebTestCase {

I think the original doc block was better for the class than the new one? It is a test class, after all.

b)

   /**
-   * Checks node revision related operations.
+   * Tests whether the correct revision text appears on the view revisions page.
    */
   function testRevisions() {
 

This is overly specific -- the test does a lot more than just that one thing. What was wrong with the original doc block here?

Similar problem here:

/**
-   * Tests changing a node's "authored by" field.
+   * Tests whether a node is authored by the currently logged in user.
    */
   function testPageAuthoredBy() {
  

So... Again I've stopped reading this patch...

And really, let's go back to the original intent of this issue, which was "Add missing docblocks for classes and methods in node.test". Let's just have a patch that adds any documentation blocks for classes/methods/properties that are actually *missing* in node.test (though of course we do not want doc blocks added for getInfo(), setUp(), and tearDown() methods).

Thanks!

Status:Needs work» Needs review
StatusFileSize
new2.47 KB
PASSED: [[SimpleTest]]: [MySQL] 40,491 pass(es).
[ View ]

There is only one method (testSyndicateBlock) missing a docblock (excluding the getInfo(), setUp(), and tearDown() methods), plus a mere handful of properties (for which an in-line comment may have sufficed ...) - though, strictly, should these properties be included? ;-)

Status:Needs review» Needs work

Thanks! Yes, every property and every method on every class is supposed to have a doc block, with the exception of getInfo, setUp, and tearDown on classes currently.

For member variables, you need @var:
https://drupal.org/coding-standards/docs#var

This type of documentation also doesn't tell me what this variable stores really:

+  /**
+   * For node revisions.
+   */
   protected $nodes;

I just don't know what "For node revisions" means at all... and the other documentation blocks added for member properties suffer from the same problem. The documentation needs to document what the property is being used for, and the @var tells what data type it is.

Also, please leave a blank line between previous code and each doc block. It looks like you removed some blank lines between classes, and put the member variable doc blocks right up next to each other. That is hard to scan visually.

The indentation is also wrong here:

+/**
+ * Tests whether a syndicate block is available.
+ */
   function testSyndicateBlock() {
 

StatusFileSize
new3.45 KB
PASSED: [[SimpleTest]]: [MySQL] 40,080 pass(es).
[ View ]

Revised patch attached ...

Thanks -- this is ***much*** better!

I found just a few things that I think could be improved... Also, when you attach the next patch, set the status to "needs review" to launch the test bot and alert human reviewers that there is a new patch. Thanks!

a)

class NodeRevisionsTestCase extends DrupalWebTestCase {
...
+  /**
+   * To log revision messages.
+   *
+   * @var array
+   */
   protected $logs;

I was kind of confused by this documentation, so I went and looked at the code... Maybe it could say something like: "The revision messages for node revisions created in the test."?

b)

PageEditUseCase...
   }
-
+  /**
+   * Tests that the "Syndicate" block is shown when enabled.
+   */
   function testSyndicateBlock() {

Don't take out the blank line between the methods please.

c) In NodeRevisionPermissionsTestCase:

   // Map revision permission names to node revision access ops.
  protected $map...

This also should be turned into a member variable doc block.

Status:Needs work» Needs review
StatusFileSize
new3.61 KB
PASSED: [[SimpleTest]]: [MySQL] 40,331 pass(es).
[ View ]

Changes made as requested.

Clarification please on @var tag and data types. From jhodgdon link (https://drupal.org/coding-standards/docs#var) in #55 should this patch use a Namespace (for example Drupal\user\Plugin\Core\Entity) or (merely) a data type?

So, for example, in this 7.x patch we have:

<?php
class NodeTitleTestCase extends DrupalWebTestCase {
 
/**
   * A user with permission to create and edit content and to administer nodes.
   *
   * @var object
   */
 
protected $admin_user;
?>

Whilst (for an equivalent entry) in 8.x we have:

<?php
class NodeBlockFunctionalTest extends NodeTestBase {
 
/**
   * An administrative user for testing.
   *
   * @var \Drupal\user\Plugin\Core\Entity\User
   */
 
protected $adminUser;
?>

Status:Needs review» Reviewed & tested by the community

This is a 7.x issue, so "@var object" is correct for 7.x -- the classes in the 8.x patch do not exist in 7.x (the 8.x docs are correct for 8.x). Thanks for asking for clarification!

So, the new patch looks good, thanks!

By the way, when you're making a large-ish patch that is just fixing a few things from the previous patch, it's nice to include an interdiff file (don't make one now, but next time):
https://drupal.org/documentation/git/interdiff

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Whoops. I made another commit to node.test today on the 7.x branch (an earlier issue), and this one doesn't apply any more. Needs a reroll.

Status:Needs work» Needs review
StatusFileSize
new3.11 KB
PASSED: [[SimpleTest]]: [MySQL] 40,525 pass(es).
[ View ]

Rerolled ...

Assigned:Unassigned» jhodgdon
Status:Needs review» Reviewed & tested by the community

Thanks! I'll get this committed soon.

Assigned:jhodgdon» Unassigned
Status:Reviewed & tested by the community» Fixed

Committed to 7.x. Thanks to all who helped on this one!

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

Removing myself from the author field so I can unfollow. --xjm