There are some classes in core/modules/search/lib/Drupal/search/Tests that have docs headers for the class that do not reflect what the class actually tests. And some where the class header is correct but the getInfo() information is incorrect. Most likely they are also wrong in D7.

Here's an example:

/**
 * Test search_simplify() on every Unicode character, and some other cases.
 */
class SearchPreprocessLangcodeTest extends SearchTestBase {
...
  public static function getInfo() {
    return array(
      'name' => 'Search preprocess langcode',
      'description' => 'Check that the hook_search_preprocess passes the correct langcode from the entity.',

Classes affected:
SearchAdvancedSearchFormTest - no class doc header at all
SearchCommentTest - class docs header is correct, getInfo is incorrect
SearchMatchtest - no class doc header at all
SearchPreprocessLangcodeTest - getInfo is correct, class docs header is incorrect
SearchRankingTest - no class doc header at all
SearchTestBase - no class doc header at all

CommentFileSizeAuthor
#17 search-test-missing-docs-headers-2169065-17.patch1.51 KBSidneyGijzen
#17 search-test-missing-docs-headers-only-backport-2169065-17-do-not-test.patch1.26 KBSidneyGijzen
#12 changed_classdocheaders_and_getinfos-2169065-12.patch3.73 KBnadja.jury
#6 search-test-classes-have-missing-inaccurate-docs-headers-2169065-4-6--interdiff.txt727 bytesdanmuzyka
#6 search-test-classes-have-missing-inaccurate-docs-headers-2169065-6.patch3.76 KBdanmuzyka
#4 changed_classdocheaders_and_getinfos-2169065-4.patch3.32 KBnadja.jury
#2 changed_classdocheaders_and_getinfos-2169065-2.patch18.4 KBnadja.jury
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nadja.jury’s picture

nadja.jury CreditAttribution: nadja.jury commented
Assigned: Unassigned » nadja.jury

Assigning to myself so I can work on it :)

nadja.jury’s picture

nadja.jury CreditAttribution: nadja.jury commented
Assigned: nadja.jury » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
changed_classdocheaders_and_getinfos-2169065-2.patch18.4 KB

Hello,

I'm new to Open Source, having only used it for around a week and a half. I've only used Drupal for 2 days now, and have very rarely used PHP before, so I do not have the experience that others on this website may have. I'm still in high school.

However, attached is a patch addressing the issues stated in the issues summary. I have checked every file (not only the ones specified) to make sure the class doc headers and the getInfo descriptions match up correctly and reflect what the class actually tests.

:)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Hey, excellent work! And welcome to the Drupal open source community! Over the years, we have had quite a few younger contributors, and I would never have guessed -- and no one has to know if you don't want to tell people. :)

A few things could be improved in your patch:

a)

+ * Verify if the search form block is available.
  */
 class SearchBlockTest extends SearchTestBase {

Verify if ===> Verify that
(same thing in the getInfo)

c) Our documentation standards (not that you could necessarily tell from reading our code!) require that class one-line descriptions start with a verb like "Verifies" instead of "Verify". See
https://drupal.org/coding-standards/docs#classes
This applies to several of the headers.

d) Also the one-line descriptions should end in ".", which applies to several of the headers.

e) You made a lot of changes of "Tests" to "Verifies". I don't really think these were a good idea for the class documentation headers. Most of the tests in Core have descriptions that start with "Tests", and I think we should try to stay consistent. The getInfo text can have Verify or Verifies in it, but I think it would be better just to leave them as they are. Every time a patch gets made and committed, you run the risk of conflicting with a patch someone is making on another issue, and causing them problems. So if a change is not really necessary, it's best to just leave it out.

f)

+ *Indexes content and queries it.
+ */
 class SearchMatchTest extends SearchTestBase {

Missing a space between * and Indexes. Also applies to SearchRankingTest. And SearchTestBase.

g)

+ * Verifies search index info is updated properly when nodes are removed or
+ * updated.
  */
 class SearchNodeUpdateAndDeletionTest extends SearchTestBase {

Classes need a one-line, 80-character-line maximum description. Also applies to SearchPreprocessLangcodeTest.

nadja.jury’s picture

nadja.jury CreditAttribution: nadja.jury commented
Status: Needs work » Needs review
FileSize
changed_classdocheaders_and_getinfos-2169065-4.patch3.32 KB

Hiya,

First of all, thank you!!

I've checked out all the files except for the 6 specified in the issue summary, so only those 6 have been changed and are part of the patch now. I've followed your suggestions for those 6 files as well. I've attached the new patch now. Thank you again! :)

danmuzyka’s picture

danmuzyka CreditAttribution: danmuzyka commented
Assigned: Unassigned » danmuzyka

Assigning to myself to review.

danmuzyka’s picture

danmuzyka CreditAttribution: danmuzyka commented
Assigned: danmuzyka » Unassigned
FileSize
search-test-classes-have-missing-inaccurate-docs-headers-2169065-6.patch3.76 KB
search-test-classes-have-missing-inaccurate-docs-headers-2169065-4-6--interdiff.txt727 bytes

Hi nadja.jury!

Thanks for submitting this updated patch! I took a look and most of your changes look good to me. One thing I noticed is that there are still a number of methods in each class that are missing documentation, but I think that may be out of the scope of this particular ticket, so I didn't change those. I noticed some inconsistency in the SearchPreprocessLangcodeTest class: the class doc header did not match the description returned by getInfo(), but I think it makes sense in this case for them to match.

I've attached an updated patch, as well as an interdiff showing the difference between your patch and the updated one I'm posting.

Could someone review my changes?

Thanks!
Dan

The last submitted patch, 6: search-test-classes-have-missing-inaccurate-docs-headers-2169065-6.patch, failed testing.

wiifm’s picture

wiifm
Primary language English
Location Wellington, NZ
CreditAttribution: wiifm commented

6: search-test-classes-have-missing-inaccurate-docs-headers-2169065-6.patch queued for re-testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Thanks for the new patches!

+ * Tests that the hook_search_preprocess passes the correct langcode from the entity.
  */
 class SearchPreprocessLangcodeTest extends SearchTestBase {

- You can take the first "the" out here.
- When referring to a hook or a function in documentation, it's important to put () after it, so that on api.drupal.org, it will automatically turn into a link. It also makes it a bit more readable.
- "langcode" should be "language code".
- A class or function needs a one-line summary that fits into an 80 character line. So this needs to be shortened... Maybe something like "Tests that search preprocessing uses the correct language code" would be good?

The rest is looking good!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Oh, and if you would like to add docs to test methods that have none, you can update the title and issue summary and expand this issue. That's within the spirit of the issue scope, at least. :)

nadja.jury’s picture

nadja.jury CreditAttribution: nadja.jury commented
Assigned: Unassigned » nadja.jury
Status: Needs review » Needs work

Assigning to myself (again) to fix the above problems :)

nadja.jury’s picture

nadja.jury CreditAttribution: nadja.jury commented
Assigned: nadja.jury » Unassigned
Status: Needs work » Needs review
FileSize
changed_classdocheaders_and_getinfos-2169065-12.patch3.73 KB

I've made the suggested changes to the SearchPreprocessLangcodeTest file (making both the class doc header and the getInfo "Tests that search preprocessing uses the correct language code.").

Thank you both so much!! :)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Excellent -- this is ready to go now. Thanks for the teamwork!

We're in a "only commit majors and criticals pre-alpha" week in Drupal 8, so I cannot commit this patch until the 23rd.

After it is committed, we can work on the Drupal 7 version.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Thanks again! This has been committed to 8.x.

Anyone want to backport to 7.x? In 7.x all the tests are lumped into one file (search.test) but I believe it suffers from very similar problems to the 8.x tests.

dcam’s picture

dcam CreditAttribution: dcam commented

I looked at these classes in D7 and noted a few things for anyone who may try to write a patch (this is still a good issue for novices).

First, I don't think SearchPreprocessLangcodeTest and SearchTestBase exist in D7, so those parts of the D8 patch probably don't have to be backported.

Second, SearchBlockTestCase in D7 does not have a docblock. It will have to be added. The docblock from D8 is:

/**
 * Tests the rendering of the search block.
 */

If I were working on this issue, I would probably write two patches. The first would be a straight backport of the D8 patch with a filename ending in -do-not-test.patch (see the file upload instructions). That way reviewers can see the backport by itself to compare with the changes made in D8. The second patch would be the backport plus the SearchBlockTestCase change.

SidneyGijzen’s picture

SidneyGijzen CreditAttribution: SidneyGijzen commented
Assigned: Unassigned » SidneyGijzen
SidneyGijzen’s picture

SidneyGijzen CreditAttribution: SidneyGijzen commented
Status: Patch (to be ported) » Needs review
Issue tags: -Needs backport to D7
FileSize
search-test-missing-docs-headers-only-backport-2169065-17-do-not-test.patch1.26 KB
search-test-missing-docs-headers-2169065-17.patch1.51 KB

I backported the latest patch of nadja.jury from comment #12. I also couldn't find the classes SearchPreprocessLangcodeTest and SearchTestBase.

Further I added the docblock to the SearchBlockTestCase class.

Per dcam's suggestion I created two patches; one do-not-test patch with only the backport, and one with the backport and the added docblock.

Status: Needs review » Needs work

The last submitted patch, 17: search-test-missing-docs-headers-2169065-17.patch, failed testing.

dcam’s picture

dcam CreditAttribution: dcam commented
Status: Needs work » Needs review

17: search-test-missing-docs-headers-2169065-17.patch queued for re-testing.

SidneyGijzen’s picture

SidneyGijzen CreditAttribution: SidneyGijzen commented

Thanks dcam for re-queue-ing. I just ran the test Programmatic Form Submission (the one that failed with the Testbot) locally and it ran without any errors...

dcam’s picture

dcam CreditAttribution: dcam commented

No problem. I could see that the failure was unrelated.

Thanks for your work on this! It's exciting to see new contributors working on these issues. I'm glad mgifford's blog post inspired some people to get involved. I think working on things like backports is a great way to get started becoming a contributor. You kind of get an easy initiation into the community's workflow. Plus you tend to see your work getting committed faster (sometimes) because most of the lengthy debate over how things should be done is over once committed to D8. This is how I started out doing core contributing and I believe a lot of new contributors could benefit from the same approach.

I'll try to review the patch sometime tonight. Good luck!

Status: Needs review » Needs work

The last submitted patch, 17: search-test-missing-docs-headers-2169065-17.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Reviewed & tested by the community

That test failure was a "couldn't find the cache table" and was unrelated to this patch. The patch is fine. Thanks!

SidneyGijzen’s picture

SidneyGijzen CreditAttribution: SidneyGijzen commented

@jhodgdon: cool. Thanks!

@dcam: Thanks for the kind words. I must say it is indeed encouraging to see a couple of patches accepted within a short period of time.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

@MF82: I maintain both the D8 Core Search module and the API documentation for Drupal Core. If you make patches in either of those areas (or both, as this one is), my aim is usually to review them within a day or two. I cannot speak for other components of Core... but I would love some more help with patches in the search.module or documentation component, any time! Thanks! (There are several issues in search.module currently waiting for backport to Drupal 7, for instance, and quite a lot of Drupal 8 API docs issues.)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 7.x.

  • Commit 1150ea8 on 7.x by jhodgdon: 
    Issue #2169065 by MF82, dcam, nadja.jury, danmuzyka: Fix up some docs in...

Status: Fixed » Closed (fixed)

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