Fix up some search tests [#2183951]

On #2179081: [meta] Inventory search.module tests. File issues to address test holes. several minor issues were identified with Search module tests, involving improving documentation, combining test classes, and similar fixes that don't actually involve changing what is tested. This issue is to fix them up.

The issues to be addressed here:

a) SearchKeywordsConditions test class needs better one-line and getInfo() documentation of what it actually does, which is that it verifies that a plugin can override the isSearchExecutable() method to allow searching without keywords set, and that GET query parameters are made available to plugins during search execution.

b) SearchPageOverrideTest needs better docs -- it says it tests hook_search_page() which doesn't even exist any more. What it actually does: verifies that a plugin can override the buildResults() method to control what the search results page looks like.

c) SearchNumberMatchingTest and SearchNumbersTest should be combined, because they are testing pretty much the same thing with different data.

d) The method SearchNodeAccessTest::testPhraseSearchPunctuation() should be moved elsewhere, because it has nothing to do with node access, and we should then delete this class. If it can't be combined into another class easily, we could also just rename the class to something more appropriate and update its documentation.

Comment	File	Size	Author
#8	Fix-up-some-search-tests-2183951-8.patch	8.07 KB	rosinegrean
#8
#3	Fix-up-some-search-tests-2183951-3.patch	9.71 KB	rosinegrean
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

rosinegrean CreditAttribution: rosinegrean commented 30 January 2014 at 16:17

I can work on this.

Comment #2

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 30 January 2014 at 16:29

That would be great! Go ahead and assign the issue to yourself, and post here or find me in IRC in #drupal-contribute if you have questions.

Comment #3

rosinegrean CreditAttribution: rosinegrean commented 30 January 2014 at 18:01

Status:

Active

» Needs review

File	Size
Fix-up-some-search-tests-2183951-3.patch	9.71 KB

Comment #4

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 30 January 2014 at 18:10

Wow, that was fast! Thanks for the patch!

This is pretty good, but a few things need to be addressed:

a) All classes, methods, etc. need a one-line, one-sentence description that fits within an 80-character line. Then you can skip a line and put a longer description in there if needed.

b) I see that you got rid of SearchNumberMatchingTest, but I don't think we wanted to lose the test coverage that it was providing. The test cases need to be added into SearchNumbersTest, unless you think they are all redundant?

+/**
+ * @file
+ * Definition of Drupal\search\Tests\SearchNodePunctuationTest.
+ */

This is using an out-of-date standard. We now prefer "Contains ..." instead of "Definition of", and also all namespaced class names need to start with \.

FYI, the standards page for writing API docs is:
https://drupal.org/coding-standards/docs

Comment #5

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 30 January 2014 at 18:11

Status:

Needs review

» Needs work

Comment #6

rosinegrean CreditAttribution: rosinegrean commented 30 January 2014 at 19:40

a) Ok

b) As far as i've seen the test is the same, with different data. Except the dash check, all are already done.

c) Should i replace "Definition of" in all of the test files ?

Comment #7

jhodgdon

she/her

English

CreditAttribution: jhodgdon commented 30 January 2014 at 19:56

Regarding the numbers tests... I took a closer look and it looks like the two tests are slightly different actually.

The SearchNumbersTest is verifying that a bunch of different types of numbers can be searched for, individually.

The SearchNumberMatchingTest is verifying that a bunch of different-looking numbers all match each other when searched for.

So I guess that the two tests are different. Maybe what we should do, in that case, is leave them both there and update the documentation. SearchNumbersTest maybe should say:
Tests that numbers with different formats can be searched.
and SearchNumberMatchingTest maybe should say:
Tests that various number formats match each other in searching.

Does that make sense?

And regarding (c), that would be good, but let's just do it for the files included in this issue for other reasons.

Thanks!