Files: 
CommentFileSizeAuthor
#5 1816162-5.patch2.63 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1816162-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Wim Leers’s picture

Assigned:Unassigned» Wim Leers
Crell’s picture

My comment from the previous thread:

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAPITest.php
@@ -0,0 +1,111 @@
+  /**
+   * Tests the ability to apply only a subset of filters.
+   */
+  function testCheckMarkup() {
+    $text = "Text with <marquee>evil content and</marquee> a URL: http://drupal.org!";
+    $expected_filtered_text = "Text with evil content and a URL: <a href="http://drupal.org">http://drupal.org</a>!";
+    $expected_filter_text_without_html_generators = "Text with evil content and a URL: http://drupal.org!";
+
+    $this->assertIdentical(
+      check_markup($text, 'filtered_html', '', FALSE, array()),
+      $expected_filtered_text,
+      'Expected filter result.'
+    );
+    $this->assertIdentical(
+      check_markup($text, 'filtered_html', '', FALSE, array(FILTER_TYPE_MARKUP_LANGUAGE)),
+      $expected_filter_text_without_html_generators,
+      'Expected filter result when skipping FILTER_TYPE_MARKUP_LANGUAGE filters.'
+    );
+    // Related to @see FilterSecurityTest.php/testSkipSecurityFilters(), but
+    // this check focuses on the ability to filter multiple filter types at once.
+    // Drupal core only ships with these two types of filters, so this is the
+    // most extensive test possible.
+    $this->assertIdentical(
+      check_markup($text, 'filtered_html', '', FALSE, array(FILTER_TYPE_HTML_RESTRICTOR, FILTER_TYPE_MARKUP_LANGUAGE)),
+      $expected_filter_text_without_html_generators,
+      'Expected filter result when skipping FILTER_TYPE_MARKUP_LANGUAGE filters, even when trying to disable filters of the FILTER_TYPE_HTML_RESTRICTOR type.'
+    );
+  }
+

This should be two separate tests. Merging two tests into a single test method is a moderately large testing faux pas.

Crell’s picture

To clarify per Wim's request, this method is testing both $expected_filtered_text and the "without html generators" situations. That's wrong. There should be one test that sets up and runs the first assertIdentical(), and a second test that sets up and runs the second assertIdentical(). Yes, there will be some overlap between those methods. That's OK. In unit tests, repeated code between methods is OK (or can be factored to a utility method); the focus is on making each test method as focused and single-purpose as possible, so that if something fails you get good fine-grained results.

And of course these should all be unit tests, not web tests, but that may not be possible until the code is refactored to be OO post-December.

Wim Leers’s picture

Gotcha. I've always scoped unit tests differently, the same way as Qt's tests do it: one test method per method/function that you want to test, and that test method will then cover all possible cases (e.g. if the method accepts an int, then it tries -1, 0, 1, 1000, and MAX_INT-1 or so). Either way works for me.

IIRC I tried to make it a UnitTestCase subclass, but that failed, IIRC because check_markup() is not available for unit tests.

Wim Leers’s picture

Status:Active» Needs review
StatusFileSize
new2.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1816162-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

1) Making this a unit test is impossible AFAICT. Unit tests imply no DB access; check_markup() relies on filter_formats() which access the DB. It is possible to work around that (e.g. by filling its static cache), but I doubt that's acceptable in core.
2) Implemented what you asked for in #3. Also fixed a typo.
3) Could you please point me to docs that state what you said in #3 is indeed required for Drupal core tests? If not, we should create that documentation, or otherwise others might do it the same way I did it.

alansaviolobo queued 5: 1816162-5.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 5: 1816162-5.patch, failed testing.