Carbon: Test issue

[e200]

Hello guys, I would like to know how you test protected methods like this:

/**
     * Update properties after removing built-in filters.
     *
     * @return void
     */
    protected function updateInternalState()
    {
        if (!$this->hasFilter(static::END_DATE_FILTER)) {
            $this->endDate = null;
        }
        if (!$this->hasFilter(static::RECURRENCES_FILTER)) {
            $this->recurrences = null;
        }
    }

Maybe this library can help you: https://github.com/e200/MakeAccessible.

[imbrish]

Hey! We test these methods like that https://github.com/briannesbitt/Carbon/blob/master/tests/CarbonPeriod/FilterTest.php#L43-L59 :)

I took a glance at your repo. From my point of view only class public api or behavior should be tested, not the internal logic. Doing that ties your tests to an implementation making any refactorings and improvements way more difficult than they need to be.

I can see cases when one would want to check some protected stuff. But I don't think there is such a need in Carbon. Thus it may not be the best place to shamelessly plug your package :P

[e200]

Thanks for your opinion. 🙂

[imbrish]

Sure and don't get me wrong - I think it's a right way to spread the word. Just try to find a project with an actual need for the functionality, preferably already using reflection in tests, and provide some actual code making test suite cleaner and/or better. Good luck!

[e200]

I'm not getting wrong, it's a question on best pratices and you're right.

I'll do that, but seems like a project that needs a package like this is poorly designed.

Like my old projects.

[kylekatarnls]

@e200 This is a very library. Here are some of my opinions you can consider or not:

• Unit tests must remain as simple as possible. So you enter your unit tests and you understand what you get with each method. That's why I would prefer a library that tell explicitly what it does:

MakeAccessible::callProtectedMethod($peopleGreeter, 'greet', $person);

Here where people read the code, they understand: "ok greet is not a method a can access from the public API, it's protected.
The side-effect of Make::accessible($peopleGreeter) is that your tests will contains: $accessiblePeopleGreeter->greet($person) if I find this in tests of a library, then I think I can use greet() this way in my app.

• Then, the purpose of unit tests is to represent real usages. So speaking about protected method, the way to use it in real cases is to extends the parent class. So that's the way I test protected methods. I ask myself "why is this protected?", if I don't find a reason, then it should be private and not tested as a stand-alone method. Then if I find a way to extend it, I write it in unit tests:

// I create this class in a features directory in my "tests" directory
class PeopleSuperGreeter extends PeopleGreeter // class from your README
{
    public function superGreet(Person $person)
    {
        if ($person->isDoctor()) {
            return 'Hi Dr. ' . $person->name();
        }
        return parent::greet($person);
    }
}
// Then in a TestCase, I test the 3 cases:
$this->assertSame('Hi Dr. Frenkenstein', (new PeopleSuperGreeter)->superGreet(new Person('doctor', 'Frenkenstein'));
$this->assertSame('Hi Mrs. Robinson', (new PeopleSuperGreeter)->superGreet(new Person('woman', 'Robinson'));
$this->assertSame('Hello Mr. Sandman', (new PeopleSuperGreeter)->superGreet(new Person('man', 'Sandman'));

• You test your library only on PHP 7.0 and 7.1 on Travis, Carbon version 1 need to work from 5.3 to 7.2 and I did not check every version but your library does not support PHP 5.3. If you can support easily, it will make it compatible with more project. If it's too much effort just drop it but specify in your composer.json the exact range of PHP compatibility you support. So users won't discover incompatibility on runtime but sooner during the install.

[e200]

@kylekatarnls thanks for share your opinion and suggestions, I'll work on it to make it more explicit based on your point of view and you're right about the support version.