[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:

• [x] action
• [x] addon
• [x] autoload
• [x] cache
• [x] command
• [x] config
• [x] controller
• [x] database
• [x] debug
• [x] escaper
• [x] event
• [x] exception
• [x] filter
• [x] form
• [x] generator
• [x] helper
• [x] i18n
• [x] log
• [x] mailer
• [x] plugin
• [x] request
• [x] response
• [x] routing
• [x] service
• [x] storage
• [x] task
• [x] test
• [x] user
• [x] util
• [x] validator
• [x] view
• [x] widget
• [x] 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. https://github.com/FriendsOfSymfony1/symfony1/blob/d9a1684cf3cde16a812eaed75dbc5ea0e8d55863/test/unit/command/sfCommandOptionTest.php#L89-L90

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

https://github.com/FriendsOfSymfony1/symfony1/blob/2826410bbfdb8dd69db968b9c44ca2550a38a9da/test/unit/routing/sfPatternRoutingTest.php#L546

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

The findRoute method calls getRouteThatMatchesUrl protected method.

https://github.com/FriendsOfSymfony1/symfony1/blob/2826410bbfdb8dd69db968b9c44ca2550a38a9da/lib/routing/sfPatternRouting.class.php#L368-L411

https://github.com/FriendsOfSymfony1/symfony1/blob/2826410bbfdb8dd69db968b9c44ca2550a38a9da/lib/routing/sfPatternRouting.class.php#L474-L487

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

https://github.com/FriendsOfSymfony1/symfony1/blob/2826410bbfdb8dd69db968b9c44ca2550a38a9da/lib/request/sfWebRequest.class.php#L433-L441

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

https://github.com/FriendsOfSymfony1/symfony1/blob/2826410bbfdb8dd69db968b9c44ca2550a38a9da/lib/i18n/sfI18N.class.php#L316-L322

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]

https://github.com/FriendsOfSymfony1/symfony1/issues/286#issuecomment-1857974108

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

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

[connorhu]

sfViewCacheManager::has() method with null return

https://github.com/FriendsOfSymfony1/symfony1/blob/2826410bbfdb8dd69db968b9c44ca2550a38a9da/lib/view/sfViewCacheManager.class.php#L384-L391

[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.