Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Status: Active » Needs review
xjm’s picture

Issue tags: +Novice

Good patch for a novice to review. :)

jhodgdon’s picture

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?

xjm’s picture

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.

xjm’s picture

Title: Add a docblock for NodeWebTestCase » Add 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. :)

totten’s picture

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

Noe_’s picture

FileSize
58.07 KB

Attached is a patch for review.

xjm’s picture

Status: Needs work » Needs review

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

jhodgdon’s picture

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!

Noe_’s picture

Status: Needs work » Needs review
FileSize
15.93 KB

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.

jhodgdon’s picture

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.

Noe_’s picture

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.

jhodgdon’s picture

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"?

pesgyo’s picture

Assigned: Unassigned » pesgyo

I'm working on it today.

pesgyo’s picture

Status: Needs work » Needs review
FileSize
19.31 KB

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.

pesgyo’s picture

Assigned: pesgyo » Unassigned
junedkazi’s picture

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

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

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

killtheliterate’s picture

Status: Needs work » Needs review
FileSize
27.76 KB

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

jhodgdon’s picture

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.

dermario’s picture

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.

wryz’s picture

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

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

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

dermario’s picture

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?

jhodgdon’s picture

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!

dermario’s picture

Status: Needs work » Needs review
FileSize
1.2 KB

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.

dermario’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs backport to D7
jhodgdon’s picture

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.

rootwork’s picture

Status: Needs work » Needs review
FileSize
1.33 KB

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

rootwork’s picture

Issue tags: +SprintWeekend2013

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

xjm’s picture

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

Trailing whitespace. :)

Looks like a straightforward cleanup otherwise!

jhodgdon’s picture

Status: Needs review » Needs work
rootwork’s picture

Status: Needs work » Needs review
FileSize
1.33 KB

Removed the trailing space.

jhodgdon’s picture

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

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

jhodgdon’s picture

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" in the UI text and help is done.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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

xjm’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs backport to D7, +SprintWeekend2013
xjm’s picture

Status: Needs review » Reviewed & tested by the community
jhodgdon’s picture

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.

bdgreen’s picture

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?

jhodgdon’s picture

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?

bdgreen’s picture

Status: Needs work » Needs review
FileSize
1.14 KB

One small patch ... ;)

jhodgdon’s picture

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).

jhodgdon’s picture

Status: Needs review » Patch (to be ported)

sorry, forgot status

bdgreen’s picture

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

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

jhodgdon’s picture

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.

bdgreen’s picture

Status: Needs work » Needs review
FileSize
16.52 KB

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

jhodgdon’s picture

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!

bdgreen’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

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? ;-)

jhodgdon’s picture

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() {
  
bdgreen’s picture

Revised patch attached ...

jhodgdon’s picture

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.

bdgreen’s picture

Status: Needs work » Needs review
FileSize
3.61 KB

Changes made as requested.

bdgreen’s picture

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:

 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:

class NodeBlockFunctionalTest extends NodeTestBase {

  /**
   * An administrative user for testing.
   *
   * @var \Drupal\user\Plugin\Core\Entity\User
   */
  protected $adminUser;
jhodgdon’s picture

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

jhodgdon’s picture

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.

bdgreen’s picture

Status: Needs work » Needs review
FileSize
3.11 KB

Rerolled ...

jhodgdon’s picture

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

Thanks! I'll get this committed soon.

jhodgdon’s picture

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.

Anonymous’s picture

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

Andrew Answer’s picture

Issue summary: View changes
Issue tags: -Needs backport to D7, -Needs reroll