FAIL.

This issue fails for several reasons:

Core contains bugs, not covered by tests. (Someone managed to change check_markup()'s function signature, but not all instances throughout core.)
This test does not fit into any module's .test file, but in parallel, it fits into all related module's .test files.
I can only select one component for this issue, while the bug, as well as the test, affects several modules.
This is where unit tests end, and inter-dependent functionality needs to be tested. Comment relies on Node, Search relies on Comment, Comment relies on Filter module. FWIW, check_markup() is invoked wrongly, and check_markup() belongs to filter.module, which is why this issue is assigned to component 'filter.module' (well, also to have it in my watchlist).

Last, but not least, this patch should fail and return 2 errors. When that happens, I'll add the two bits that should make it pass.

Comment	File	Size	Author
#18	drupal7-sun.randomName.patch	1.11 KB	sun
#18
#15	drupal.filter-comment-search-15.patch	7.07 KB	dmitrig01
#15
#14	drupal.filter-comment-search-14.patch	6.19 KB	dmitrig01
#14
#12	drupal.filter-comment-search-11.patch	6.86 KB	sun
#12
#11	drupal.filter-comment-search-10.patch	7.02 KB	sun
#11
#7	drupal.filter-comment-search-6.patch	6.98 KB	sun
#7
#6	drupal.filter-comment-search-5.patch	6.95 KB	sun
#6
#4	drupal.filter-comment-search-4.patch	6.95 KB	sun
#4
	drupal.filter-comment-search.patch	3.01 KB	sun

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

sun

German

Karlsruhe

CreditAttribution: sun commented 28 April 2009 at 00:04

Component:

comment.module

» filter.module

uhm.

Comment #2

28 April 2009 at 00:20

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #3

sun

German

Karlsruhe

CreditAttribution: sun commented 28 April 2009 at 00:50

Another FAIL:

- Someone updated hook_nodeapi() to be split into individual functions and managed to invoke module_invoke() in a completely weird way:

        // Fetch comments for snippet.
        $node->body .= module_invoke('comment', 'node', $node, 'update_index');
        // Fetch terms for snippet.
        $node->body .= module_invoke('taxonomy', 'node', $node, 'update_index');

File	Size
drupal.filter-comment-search-4.patch	6.95 KB

Comment #5

28 April 2009 at 01:40

Status:

Needs review

» Needs work

The last submitted patch failed testing.

File	Size
drupal.filter-comment-search-5.patch	6.95 KB

Comment #7

sun

German

Karlsruhe

CreditAttribution: sun commented 28 April 2009 at 01:57

File	Size
drupal.filter-comment-search-6.patch	6.98 KB

Now invoking cron the same way like system.test. Thanks for the pointer, chx.

Comment #8

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 28 April 2009 at 02:09

Yeah using url() with cron.php/update.php doesn't work since the test bot runs in a subdirectory apparently. I had the same problem with tests and drupalGet() with adding a permission to run update.php.

Comment #9

chx CreditAttribution: chx commented 28 April 2009 at 02:14

Status:

Needs review

» Reviewed & tested by the community

Wow, lots of bugfixes. And, nice ASCII art.

Comment #10

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 28 April 2009 at 02:15

Status:

Reviewed & tested by the community

» Needs review

+    $edit = array(
+      '1[search content]' => 1,
+      // @todo Comments are added to search index without checking first whether
+      //   anonymous users are allowed to access comments.
+      '1[access comments]' => 1,
+      // @todo Without this permission, "Login or register to post comments" is
+      //   added to the search index.  Scratch that, comment.module did not even
+      //   add anything to search results.  Aforementioned link text must be
+      //   added via node links.
+      '1[post comments]' => 1,
+    );

Should be using
DRUPAL_ANONYMOUS_RID . '[search content]' => 1, etc
Side node, I should really get back on #300993: User roles and permissions API so we don't have to use code like this in tests...

+    // Create a node.
+    $edit = array(
+      'title' => $this->randomName(),
+    );
+    $this->drupalPost('node/add/page', $edit, t('Save'));
+    // Check that the node exists in the database.

Why not use drupalCreateNode() since you're not doing anything special?

And yes, the ASCII art is funny/cool...but seriously...

Comment #11

sun

German

Karlsruhe

CreditAttribution: sun commented 28 April 2009 at 02:24

Status:

Needs review

» Reviewed & tested by the community

File	Size
drupal.filter-comment-search-10.patch	7.02 KB

Of course, the contained code to setup the check_plain() filter had a purpose. search_excerpt() strips all tags from the snippet that is displayed in the search results. If the content was formatted with the wrong text format, the HTML would be escaped.

Added this last assertion and clarified @todo 2) about the node comment link that's added to search index.

Comment #12

sun

German

Karlsruhe

CreditAttribution: sun commented 28 April 2009 at 02:29

File	Size
drupal.filter-comment-search-11.patch	6.86 KB

Incorporated all points from Dave Reid's review in #10.

Comment #13

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 28 April 2009 at 04:57

Status:

Reviewed & tested by the community

» Needs work

Since there were no steps posted to reproduce the bugs identified here (grumble, mutter), I had to reverse-engineer the test to figure it out. I came across some things when I did.

+    // Enable comments for 'page' nodes.
+    variable_set('comment_page', COMMENT_NODE_OPEN);
+    variable_set('comment_preview_page', COMMENT_PREVIEW_OPTIONAL);

There's already enough going on in this test; let's not muddy the waters more by adding comments to Pages as well; just use article. If we don't have coverage for comments on Pages, let's put it in comment.test in a separate issue, since it seems unrelated to these bugs here.

--- modules/filter/filter.test	25 Apr 2009 18:01:10 -0000	1.19
+++ modules/filter/filter.test	28 Apr 2009 02:27:59 -0000

Speaking of test locations, I realize that the bugs ultimately lie in the filter system, but the test is clearly an integration test of the search functionality, and should be relocated to search.test IMO. I can't forsee anyone ever finding this test in filter.test once all of the underlying bugs are fixed.

+    // Enable check_plain() for 'Filtered HTML' text format.
+    $edit = array(
+      'filters[filter/0]' => 1,
+      'filters[filter/1]' => 1,
+      'filters[filter/2]' => 1,
+      'filters[filter/3]' => 1,
+      'filters[filter/4]' => 1,
+    );

Why do we need to specify 1 for each of them, if we're only interested in "Escape all HTML"? Again, let's please keep the test focused only to what it needs to do to exhibit the buggy behaviour. Unless this is required, in which case, please clarify in the comment. Also, please add the "Escape all HTML" keywords to the comment as well.

+    /**
+     *                     ,____
+     *                     |---.\
+     *             ___     |    `
+     *            / .-\  ./=)
+     *           |  |"|_/\/|
+     *           ;  |-;| /_|
+     *          / \_| |/ \ |
+     *         /      \/\( |
+     *         |   /  |` ) |
+     *         /   \ _/    |
+     *        /--._/  \    |
+     *        `/|)    |    /
+     *          /     |   |
+     *        .'      |   |
+     * jgs   /         \  |
+     *      (_.-.__.__./  /
+     * @see http://drupal.org/cvs?commit=79939
+     */

+    $this->assertText($title, t('Comment body text found in search results.'));

Forgive my ignorance, but how does this prove anything? Unless I'm really tired, you're using the same text for both the title of the node and the body of the comment, so this test will pass even without this patch.