[tests] port the test system to phpunit

[connorhu]

This is an issue to track the progression.

Branch: https://github.com/connorhu/symfony1/tree/feature/phpunit
Target php version: php7.4

Components:

• action
• addon
• autoload
• cache
• command
• config
• controller
• database
• debug
• escaper
• event
• exception
• filter
• form
• generator
• helper
• i18n
• log
• mailer
• plugin
• request
• response
• routing
• service
• storage
• task
• test
• user
• util
• validator
• view
• widget
• yaml

[connorhu]

https://github.com/connorhu/symfony1/blob/feature/phpunit/tests/controller/sfWebControllerTest.php#L37-L51
sfWebControllerTest now fail because of type error.

       yield ['module/action?id=12', array(
            '',
            array(
                'module' => 'module',
                'action' => 'action',
                'id'     => 12,
            ),
        )];

:

--- Expected
+++ Actual
@@ @@
     1 => Array &1 (
         'module' => 'module'
         'action' => 'action'
-        'id' => 12
+        'id' => '12'
     )
 )

If we look at the test writer intent, then all numeric arguments must be converted to real numbers.

[connorhu]

A copy from issue #285

PHPUnit brings up the errors nicely. The first assert fails because of an acceptParameter method bug.

        $option = new sfCommandOption('foo', null, sfCommandOption::IS_ARRAY);
        $this->assertSame(true, $option->acceptParameter());

        $option = new sfCommandOption('foo', null, sfCommandOption::PARAMETER_OPTIONAL);
        $this->assertSame(true, $option->acceptParameter());

        $option = new sfCommandOption('foo', null, sfCommandOption::PARAMETER_REQUIRED);
        $this->assertSame(true, $option->acceptParameter());

        $option = new sfCommandOption('foo', null, sfCommandOption::PARAMETER_NONE);
        $this->assertSame(false, $option->acceptParameter());

Description of acceptParameter describes how it should work, but the implementation doesn't do that:

https://github.com/FriendsOfSymfony1/symfony1/blob/master/lib/command/sfCommandOption.class.php#L105-L110

I guess the implementation should something like this:

  public function acceptParameter(): bool
  {
    return self::PARAMETER_NONE !== (self::PARAMETER_NONE & $this->mode);
  }

The sfCommandOptionTest test it, but because of [] == false is true, the test don't fail.

symfony1/test/unit/command/sfCommandOptionTest.php

Lines 89 to 90 in d9a1684

	$option = new sfCommandOption('foo', null, sfCommandOption::IS_ARRAY);
	$t->is($option->getDefault(), array(), '->getDefault() returns an empty array if option is an array');

What should we do with bugs like this? Fix it in 1.5.x or we wait for 1.6.x and after I finish the integration of phpunit we fix it and backport to 1.5.x?

[connorhu]

Another "who made a mistake?" memo

symfony1/test/unit/routing/sfPatternRoutingTest.php

Line 546 in 2826410

$t->is($rCached->findRoute('/no/match/found'), null, '->findRoute() returns null on non-matching route');

Test message says: "->findRoute() returns null on non-matching route"

The findRoute method calls getRouteThatMatchesUrl protected method.

symfony1/lib/routing/sfPatternRouting.class.php

Lines 368 to 411 in 2826410

	/**
	* Finds a matching route for given URL.
	*
	* Returns false if no route matches.
	*
	* Returned array contains:
	*
	* - name: name or alias of the route that matched
	* - pattern: the compiled pattern of the route that matched
	* - parameters: array containing key value pairs of the request parameters including defaults
	*
	* @param string $url URL to be parsed
	*
	* @return array|false An array with routing information or false if no route matched
	*/
	public function findRoute($url)
	{
	$url = $this->normalizeUrl($url);
	// fetch from cache
	if (null !== $this->cache) {
	$cacheKey = $this->getParseCacheKey($url);
	if ($this->options['lookup_cache_dedicated_keys'] && $info = $this->cache->get($cacheKey)) {
	return unserialize($info);
	}
	if (isset($this->cacheData[$cacheKey])) {
	return $this->cacheData[$cacheKey];
	}
	}
	$info = $this->getRouteThatMatchesUrl($url);
	// store in cache
	if (null !== $this->cache) {
	if ($this->options['lookup_cache_dedicated_keys']) {
	$this->cache->set($cacheKey, serialize($info));
	} else {
	$this->cacheChanged = true;
	$this->cacheData[$cacheKey] = $info;
	}
	}
	return $info;
	}

symfony1/lib/routing/sfPatternRouting.class.php

Lines 474 to 487 in 2826410

	protected function getRouteThatMatchesUrl($url)
	{
	foreach ($this->routes as $name => $route) {
	$route = $this->getRoute($name);
	if (false === $parameters = $route->matchesUrl($url, $this->options['context'])) {
	continue;
	}
	return array('name' => $name, 'pattern' => $route->getPattern(), 'parameters' => $parameters);
	}
	return false;
	}

The getRouteThatMatchesUrl method returns false.

IMHO the test is wrong because the return annotation of findRoute method says:
array|false An array with routing information or false if no route matched.

[alquerci]

IMHO annotation is just a comment and comments can lie.

The only things that do not lie is the code behaviour.

The question is what kind of assertion is done with $t->is(), === or == ?

[connorhu]

During porting, I change the is method to the more type-strict assertSame method, so I look at the tester's intent in each case. However, here the tester intent is wrong.

[alquerci]

IMHO good idea, but it tends to complexity this migration.

So in this context, the source of truth is the production code.

On this example:
if return value is false
then assertFalse is the way to go.

Like you said. The test code is good, but the test message is misleading.
My message was abstract.

[connorhu]

Another "who made a mistake? / where is the bug?" memo

sfWebRequestTest.php

        $request->languages = null;
        $_SERVER['HTTP_ACCEPT_LANGUAGE'] = '';
        $this->is($request->getLanguages(), [], '->getLanguages() returns an empty array if the client send an empty ACCEPT_LANGUAGE header');

Test message says: getLanguages() returns an empty array if the client send an empty ACCEPT_LANGUAGE header

symfony1/lib/request/sfWebRequest.class.php

Lines 433 to 441 in 2826410

	public function getLanguages()
	{
	if ($this->languages) {
	return $this->languages;
	}
	if (!isset($_SERVER['HTTP_ACCEPT_LANGUAGE'])) {
	return array();
	}

Now the isset() is not enough we should check empty() too.

[connorhu]

Another "who made a mistake? / where is the bug?" memo

sfI18NTest.php

$t->is($i18n->getTimeForCulture(null), null, '->getTimeForCulture() returns null in case of conversion problem');

Test message says: ->getTimeForCulture() returns null in case of conversion problem

symfony1/lib/i18n/sfI18N.class.php

Lines 316 to 322 in 2826410

	* @return array An array with the hour and minute
	*/
	public function getTimeForCulture($time, $culture = null)
	{
	if (!$time) {
	return 0;
	}

I guess there is an implementation bug and we should return null here not 0.

An extra typo is that the phpdoc return type is not only an array but also null.

[alquerci]

#286 (comment)

$acceptLanguage = '0';
empty($acceptLanguage) // true

Does a string "0" is an empty ACCEPT_LANGUAGE header ?

[connorhu]

sfViewCacheManager::has() method with null return

symfony1/lib/view/sfViewCacheManager.class.php

Lines 384 to 391 in 2826410

	*
	* @return bool true, if there is a cache otherwise false
	*/
	public function has($internalUri)
	{
	if (!$this->isCacheable($internalUri) || $this->ignore()) {
	return null;
	}

[connorhu]

#286 (comment)

$acceptLanguage = '0';
empty($acceptLanguage) // true

Does a string "0" is an empty ACCEPT_LANGUAGE header ?

An empty array is fine when client send 0. After all, this is an invalid value.

https://github.com/connorhu/symfony1/blob/998c9a40e210f1fd9acab51aa51cd32400811ca0/tests/request/sfWebRequestTest.php#L39-L42

[connorhu]

Side note: we should deprecate and totally remove pear support (plugin component). PEAR is a deprecation hell and not working with composer.