Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of meta-issue #1310084: [meta] API documentation cleanup sprint
This is for the system module, subdirectory tests, files and sub-directories starting with A through D, including the drupal_system_listing_compatible_test
and drupal_system_listing_incompatible_test
subdirectories.
Comment | File | Size | Author |
---|---|---|---|
#34 | 1431658-34-simpletest-api-cleanup-a-to-d.patch | 119.76 KB | dcam |
#31 | simpletest-cleanup_api_a_to_d-1431658-31.patch | 27.89 KB | Albert Volkman |
#31 | interdiff.txt | 4.52 KB | Albert Volkman |
#29 | simpletest-cleanup_api_a_to_d-1431658-29.patch | 27.13 KB | Albert Volkman |
#26 | simpletest-cleanup_api_a_to_d-1431658-26.patch | 127.11 KB | Albert Volkman |
Comments
Comment #1
jhodgdonActually, we're splitting this up a bit more. Just A-D on this issue.
Comment #2
jhodgdonComment #3
xjmComment #4
connorwk CreditAttribution: connorwk commentedHere is my patch for just actions.test
My first patch :D
More to come!
Comment #5
jhodgdonhi connork, thanks for the patch!
So... First you need to read through
http://drupal.org/node/1354
(especially the part about verbs on the first line of function/class docs).
And there are some specific standards for test classes that say you should not put a documentation block on the setUp(), getInfo(), or shutDown() functions at all. Which I'm not happy about, but that was the community decision.
Comment #6
connorwk CreditAttribution: connorwk commentedOk my second try at the patch i think i understand now!
If not let me know and I will try, try again.
Comment #7
connorwk CreditAttribution: connorwk commentedOk another patch for another file!
Comment #8
connorwk CreditAttribution: connorwk commentedIGNORE PREVIOUS PATCHES THIS IS THE FINAL.
This is the final patch that I'm doing for this I've done a 3651 line patch file.
NO MORE!!!
Jk I'm a beginner and I've done what I can.
Comment #9
rgristroph CreditAttribution: rgristroph commentedThis is a big patch, but I read through all of it, nice work.
I changed a small handful of things and rolled a new patch. I think this is good, and the only changes are in comments.
--Rob
Comment #10
Gábor HojtsyJust a quick review of two common mistakes to keep you moving! I realize you did not introduce these things, but since you are changing the stuff anyway, fixing it would be an ideal scenario.
Intro comments on functions/methods should be on a single line. If you modify an existing one, "unfortunately" it would be ideally coupled with fixing the multiline-ness of it too.
Good twitter skills pay off big time here.
There might be other instances of this problem, I just found these at the top.
These and possibly others are above 80 chars. That is another typical problem (which is an attempt to avoid the multilines), but should not be used.
This is even tougher than twitter, you have much less space here...
Comment #11
jhodgdonThanks Gabor! Those two review comments are correct. See
http://drupal.org/node/1354#general (80-character lines in comments)
http://drupal.org/node/1354#functions (first-line description in one sentence)
Comment #12
iamcarrico CreditAttribution: iamcarrico commentedI went through the patch one more time and fixed the common errors from jhodgdon and Gàbor. This patch should fix the 2-line issues as well as all the 80-column issues from the previous patches.
Comment #13
iamcarrico CreditAttribution: iamcarrico commented--forgot to change the status of the issue.
Comment #14
xjmThanks @ChinggizKhan!
Updating for #1299424: Allow one module per directory and move system tests to core/modules/system. This patch will probably need a rebase.
Comment #15
xjm#13: simpletest-cleanup_api_a_to_d-1431658-13.patch queued for re-testing.
Comment #16.0
(not verified) CreditAttribution: commentedMention subdirectories
Comment #17
kid_icarus CreditAttribution: kid_icarus commentedRe-rolled. Attached is a giant interdiff as well.
Comment #18
xjmI think your interdiff is backwards. :) Edit: To be clear, that's just an observation--don't worry about making another in this case. Starting a review on this now.
Comment #19
xjmThanks @connork, @rgristroph, @ChinggizKhan, and @kid_icarus for all your work on this patch. It's a big one!
I reviewed about 20% of the patch, starting from the bottom up, but stopped there because there are just too many outstanding issues. (It's already taken me an hour reviewing that much.) Couple general observations:
Specifics from as much as I got through:
All these summaries need to be one line of fewer than 80 characters, beginning with a third-person verb. If there's additional information to add, we can add it in a subsequent paragraph (as in the case of several here include a second sentence).
The @return here should be removed. Test methods don't return anything.
Can we fit "an" before "inner pager query" here, or otherwise reword this? It's a little confusing.
The correction for the verb forms here puts these line over 80 characters, so we need to reword them to be shorter.
Let's add a comma after "valid" here.
These are not selecting anything. They are testing select query alters and tagging.
I don't think "tag" needs to be in quotes here.
"Metadata" should be one word.
These needs to be reworded to be under 80 characters.
I don't think this is testing regressions. I think it's testing to make sure that there aren't regressions... However, I think the naming of the class itself is kind of questionable, so I'm not sure what to change this to. I might look at what the
setUp()
method does and what all the methods are, and try to draw a picture from that.The rewording here is inaccurate. However, this docblock is bad to begin with, because the summary should explain what the function does, not reference something somewhere on the internet. How about replacing both these sentences with:
"Ensures that non-ASCII UTF-8 data is stored in the database properly."
Aside from the fact that "queries" is spelled incorrectly, the function isn't querying serialization tests, it's testing query serialization. So let's change this to:
"Tests query serialization."
I am pretty sure we are not ranging anything here. :) Let's make this "Tests range queries."
This contains grammatical errors. It should be:
"Confirms that range queries work and return the correct result"
The word "for" here either makes "Tests" a noun, or changes the meaning of the sentence. Simply removing the word "for" fixes this.
These need articles (a committed transaction, a LIKE query, etc.) (This likely applies elsewhere in the patch as well.)
This method still needs a one-line summary at the top above this paragraph.
How about, "Tests transaction stacking, commit, and rollback"?
I think this needs to be "works"? API is singular.
Let's get a reroll to fix these specific points, and to also apply these suggestions elsewhere in the files. Please be sure to include an interdiff from the previous patch. Thanks!
Comment #20
Albert Volkman CreditAttribution: Albert Volkman commentedComment #21
Albert Volkman CreditAttribution: Albert Volkman commentedHere's what I did (sorry I can't create an interdiff due to PSR-0ification of tests):
Comment #22
jhodgdon#21: simpletest-cleanup_api_a_to_d-1431658-21.patch queued for re-testing.
Comment #24
Albert Volkman CreditAttribution: Albert Volkman commentedRe-rolled.
Comment #25
jhodgdonThis is very close, thanks!
I reviewed the entire patch (wow!) and I found just a couple of spots that need minor clean-up and then we should be able to get this committed... Two things that could be done:
1. Make a patch that just removes these errors and do the rest in a follow-up.
2. Make a patch that fixes these errors.
Either way... please provide an interdiff! :)
Anyway, here are the commit blockers:
a) This one-liner needs some attention (it doesn't quite make grammatical sense):
b)
Needs a blank line in there.
c)
Our test coding standard is that setUp(), tearDown(), and getInfo() methods do not have documentation.
d) And in the same file:
Address should not be capitalized.
e)
Needs a one-line summary, maybe something like "Tests cache get and set.".
f)
We don't have an @global docs tag... not sure where those lines came from? Just remove them. There are some elsewhere in the patch as well in files:
+++ b/core/modules/system/lib/Drupal/system/Tests/Common/AlterTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Common/HttpRequestTest.php
g)
assert -> asserts in the last line for verb agreement.
h) There were a couple of places where in-code comments' verb tense was changed and it shouldn't be, such as:
i)
Actually, "insert" here wasn't a verb... This should probably be "Tests insertion for database default values".
j)
First-line for function doc -- needs to start with a verb. (there are two in this file)
k)
These look like form constructors/submit handlers and should be documented as in http://drupal.org/node/1354#forms
l)
We need to be consistent here... Elsewhere in this patch, I see "Ajax" and here it's "AJAX". I think the proper term for Drupal is "Ajax".
m)
Maybe "Provides" instead of "Performs"?
n)
Performs is not the right verb here either... Actually I think this are form constructur/submission handlers and should be documented as in http://drupal.org/node/1354#forms -- same with the multi-step and chained forms and mock form later in this file.
o) same file:
form_test_mock_form() needs ()
p) same file
How about "Does nothing"?
q)
Needs to start with a verb... How about "Provides a landing page..."? Also it's probably a page callback, so it should probably be "Page callback: Provides a landing..." and should have @see common_test_menu() [assuming it really is a page callback].
Same applies to the goto_land_fail() function just after this one.
r)
Maybe "Provides a theme function..."?
Comment #26
Albert Volkman CreditAttribution: Albert Volkman commentedThanks for the review @jhodgdon! This patch is a monster.
l) I've actually seen both. Even k) has inconsistencies. Perhaps this should be handled in another issue?
New patch and interdiff attached!
Comment #27
jhodgdonl) Our standard for Drupal is to call it "Ajax", so a line that changes that to AJAX in the patch isn't too cool. But yes, let's check that in a follow-up.
k) We can do the form constructor docs in a follow-up patch on this issue... looks like you took care of them in your new patch anyway.
Anyway... this looks good! I'll commit it shortly.
Comment #28
jhodgdonCOMMITTED!!! Many thanks to all who contributed to this massive patch!
So... Let's go back and take a look at what else needs updating... well in D8 we're now talking about core/modules/system/lib/Drupal/system/Tests/, so let's see what needs to be cleaned up still in Ajax, Batch, Bootstrap, Bundle, Cache, Common, and Database directories...
a) Standardize on Ajax not AJAX.
b) Class docs one-liners that need attention: Note that ones that say "Tests for the ..." can probably be fixed by just removing the "for":
- Ajax/AjaxTestBase: Should start with verb, like: "Provides a base class for Ajax tests" maybe?
- Ajax/MultiformTest: needs to be <= 80 characters
- Batch/PageTest: start with verb
- Batch/ProcessingTest: start with verb
- Batch/PercentagesUnitTest: start with verb
- Bootstrap::OverrideServerVariablesUnitTest: start with verb
- Bundle/BundleTest - verb tense
- Common/HtmlIdentifierUnitTest: Should start with verb
- Common/SchemaTest: should start with verb
- Common/TableSortExtenderUnitTest: verb tense
- Common/XssUnitTest: should start with a verb
- Database/DatabaseTestBase: should start with verb
- Database/DeleteTruncateTest: should start with verb
- Database/InsertLobTest: Should start with a verb
- Database/SelectSubqueryTest: should start with verb
- Database/SelectTableSortDefaultTest: doesn't end in .
- Database/UpdateTest - class one-liner doesn't make sense, should be "Tests the update query builder" I think?
c) Method docs one-liners that need attention:
- Bundle/BundleTest::testBundleRegistration() - verb tense
- Cache/CacheTestBase::assertCacheExists(), assertCacheRemoved() - verb tense and what's that "or" word?
- Cache/GenericCacheBackendUnitTestBase:: [most methods] - verb tense, and some need one-line summary
- Cache/GenericDatabaseBackendUnitTestBase:: several methods have no docs
- Cache/MemoryBackendUnitTest::createCacheBackend() - needs docs
- Common/AddFeedTest::testFeedIconEscaping - verb tense, one sentence, second sentence in new paragraph
- Common/FormatDateTest::testFormatDate() - should start with verb
- Common/SchemaTest::testSchema(), tryInsert() - needs docs
- Common/SchemaTest::assertFieldAdditionRemoval(), assertFieldCharacteristics() - verb tense
- Common/SimpleTestErrorCollectorTest::error - should start with a verb
- Common/TableSortExtenderUnitTest::testTableSortInit() - verb tense
- Common/UrlTest::hasClass() - needs docs
- Database/InsertTest::testInsertLastInsertID() - verb tense
- Database/MergeTest::testMergeUpdateExplicit() - needs to be <= 80 characters
- Database/SelectComplexTest::testHavingCountQuery() - doesn't have docs
- Database/SelectTableSortDefaultTest::testTableSortDefaultSort() - needs to be <= 80 characters
d) Other grammar/wording/spelling/misc issues
- Ajax/MultiformTest::testMultiForm() - "Tests that pages ... works correctly" should be "work".
- Batch/ProcessingTest::assertBatchMessages() - needs blank line between param/return
- Common/CascadingStylesheetsTest::testReset - reseting should be resetting I think?
- Database/SelectTest::testSelectDuplicateAlias() - doesn't quite make sense to me, maybe should say "are renamed when they are duplicates"?
Comment #29
Albert Volkman CreditAttribution: Albert Volkman commentedI *think* I got 'em all :)
Comment #30
jhodgdonThanks, this patch is mostly excellent! The only things I think ought to be fixed are a few grammar/punctuation nitpicks:
a) More a/an/the in the following lines:
b) This one needs to be split into summary (up to comma) and a separate paragraph. Or else the comma needs to be changed to ; -- and it also needs "a" added: "Gets a backend to test.":
c)
Seems like it would be better without "the".
d) Needs @param docs added:
e) Double negative, whoops!
Comment #31
Albert Volkman CreditAttribution: Albert Volkman commentedFixed!
Comment #32
jhodgdonThanks, I think this one's ready to go! I'll commit it shortly.
Comment #33
jhodgdonCommitted to 8.x -- thanks to all who worked on this!!
I guess we should probably try to port as many of the updates as possible to 7.x now. The tests are not in the same files, but it may be possible to port them anyway.
Comment #34
dcam CreditAttribution: dcam commentedBackported #26 and #31 to D7.
Comment #34.0
dcam CreditAttribution: dcam commented.
Comment #35
jhodgdonThese issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.
Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!