[TYPO3-core] [TYPO3-dev] Unit testing suggestions (was: RFC #13858: IS cannot not index files if absRefPrefix is set and indexExternalURLs is not)

[Oliver Klee]

Hi Dmitry,

as promised, here are some suggestions on how to improve the unit tests.
 I hope that they are useful to you. :-)

I've posted this message so that follow-ups go to the dev newsgroup.


+/**
+  * This class contains unit tests for the indexer

I propose mentioning the *name* of the tested class here.

+class tx_indexedsearch_indexer_testcase extends tx_phpunit_testcase {
+
+	/**
+	 * Indexer instance
+	 *
+	 * @var tx_indexedsearch_indexer
+	 */
+	protected $indexer;

I propose to rename this variable to $fixture (because "fixture" in unit
tests usually is an instance of the tested class).

+	/**
+	 * Sets up the test
+	 *
+	 * @return void
+	 */
+	public function setUp() {

Usually, PhpDoc comments for setUp and tearDown are not necessary
because the tasks of these two functions are clearly defined.

+	/**
+	 * Explicitly cleans up the indexer object to prevent any memory leaks
+	 *
+	 * @return void
+	 */
+	public function tearDown() {
+		unset($this->indexer);
+		if ($this->temporaryFileName) {
+			@unlink($this->temporaryFileName);
+		}
+	}

This looks good. Unsetting the indexer is really good here because it
helps the garbage collector.

+	/**
+	 * Checks that non-existing files are not returned
+	 *
+	 * @return void
+	 */
+	public function testNonExistingLocalPath() {

1. Test names tend to become quite long. Using the @test annotation
instead of the "test*" prefix can help keeping the test function names
short.

2. PhpDoc for test functions (description, @return ...) usually is not
needed because test functions are not expected to be called from the
outside. The only exceptions of this are:
   - @test
   - @see if you want to point to a function name or a bug report URL
   - @param/@return if you are using data providers (which you are not)


+	/**
+	 * Checks that non-existing files are not returned
+	 *
+	 * @return void
+	 */
+	public function testNonExistingLocalPath() {
+		$html = 'test <a href="' . md5(uniqid('')) . '">test</a> test';
+		$result = $this->indexer->extractHyperLinks($html);
+
+		$this->assertEquals(1, count($result), 'Wrong number of parsed links');
+		$this->assertEquals($result[0]['localPath'], '', 'Local path is
incorrect');
+	}

The test name is what's visible when running the tests. When a test
fails, the test function name (and the assertion message, if you decide
to use any) should tell exactly what piece of behavior is broken. So the
function name should include the following three things:

- what function/class is tested (usually, it's a good idea to start the
test function name with the name of the tested function so the tests are
easier to find)
- the tested conditions/data
- the expected result or behavior

Suggested names for the test:

extractHyperLinksForNonExistingLocalPathReturnsEmptyStringAsLocalPath

... and change the two asserts to one:

$this->assertEquals(
	array(0 => array('localPath' => '')),
);


If you want to explicitly test both aspects (the number of returned
values and the string value), I propose using two separate tests with
good names instead of one test. This makes is easier to see at a glance
which part is broken (without having to read the test code), especially
if only one of the two tests fail.

As a rule of thumb, the vast majority of tests that test exactly one
piece of behavior should contain only one assert statement.


+	/**
+	 * Tests that a path with the absRefPrefix returns correct result
+	 *
+	 * @return void
+	 */
+	public function testLocalPathWithAbsRefPrefix() {
+		$absRefPrefix = '/' . md5(uniqid(''));
+		$html = 'test <a href="' . $absRefPrefix . 'index.php">test</a> test';
+		$savedPrefix = $GLOBALS['TSFE']->config['config']['absRefPrefix'];
+		$GLOBALS['TSFE']->config['config']['absRefPrefix'] = $absRefPrefix;
+		$result = $this->indexer->extractHyperLinks($html);
+		$GLOBALS['TSFE']->config['config']['absRefPrefix'] = $savedPrefix;
+
+		$this->assertEquals(1, count($result), 'Wrong number of parsed links');
+		$this->assertEquals($result[0]['localPath'], PATH_site . 'index.php',
'Local path is incorrect');
+	}

"Returns the correct result" is a very general statement. :-) Better
would be what kind of result is returned, e.g. in relation to the
absPrefix, or just exactly the nonempty value from the href, or
whatever. This should go into the test name, not into the comment.

Concerning the saved prefix: I recommend moving the backup and restore
code to setUp and tearDown (that's where this kind of code belongs).

Okay, so much for today.


Oli
-- 
Certified TYPO3 Integrator | TYPO3 Security Team Member