Statically determine if a cache adapter is enabled. #831

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@blainesch
Member

In Adaptable::enabled() it first asks for an instance of the adapter, which might be fine with other adaptables but for caching this does not work correctly since you could get a fatal error.

Fatal error: Class 'Memcached' not found
@jails
Collaborator
jails commented Feb 22, 2013

Imo we shouldn't use the Mocker in core tests. The current syntax of the mocker seems messy and it'll be a real pain to change the API if a lot of tests already use it. And this is more "readable" imo:

public function testNotCreateCacheWhenTestingEnabled() {
    $adapter = new MockCallable();
    Cache::config(array(
        'nocache' => array(
            'adapter' => $adapter
        )
    ));
    Cache::enabled('nocache');
    $this->assertEmpty($adapter->call);
}

Ps:
Since there's already a lot of "Mocking" implementations why not moving the mocker on its own repository and let people choosing how they wan't to write tests ?

@ericcholis

Thank you for this @BlaineSch I had a really crappy environment based hack to account for this in a few projects. This is much nicer.

@blainesch
Member

@jails if I was going to create a hard coded "mock" file it would be static and likely only used for a single test, could potentially have bugs, and the only line that would change is the class used on 656, and lastly, if the syntax for applying filters changes I doubt this test would be the biggest issue core had. The class Mocker solves a lot of these issues.

If this had been rejected by core it would be in its own library, but it wasn't. Personally I believe having amazing built-in testing tools is a good thing, and there is lots of room to grow in this area.

@gwoo
Member
gwoo commented Feb 22, 2013

If this is in the CacheTest, why does the test use a session adapter mock? Should this be in the Adaptable class?

@blainesch
Member

@gwoo the MockPhp was the only currently created "mock" file with an enabled() method. The result of this is not used anywhere so I didn't think it was all that important. It could potentially be replaced with a real adapter.

There is currently an issue with not being able to create methods on a blank class with Mocker, and I have a possible solution, but it's not code yet. Being able to add a enabled() method on a blank class would be nice.

If this is better in Adaptable I could move it there and modify the test. There may have been a good reason why Adaptable::enabled() got an instance instead of the class. I'll open up a new pull request with the different implementation.

Blaine Schmeisser Statically determine if a cache adapter is enabled.
In `Adaptable::enabled()` it first asks for an instance of the adapter, which might be fine with other adaptables but for caching this does not work correctly since you could get a fatal error.

~~~
Fatal error: Class 'Memcached' not found
~~~
575c0db
@blainesch blainesch referenced this pull request Feb 22, 2013
Merged

Bug/adaptable enabled #832

@jails
Collaborator
jails commented Feb 22, 2013

@BlaineSch sorry I missed to say that MockCallable is already an existing class of the core and the following:

use lithium\tests\mocks\core\MockCallable;

should be enough for making the above test works.

Agreed, amazing built-in testing tools is a good thing but I think lithium is enough flexible for supporting all of them. I don't think the core need to impose something specific here. I'll be better to be "open minded" and delegating this to external libraries, and let people choosing that best match imo.

@nateabele
Member

Merged #832 instead.

@nateabele nateabele closed this Feb 24, 2013
@blainesch blainesch deleted the unknown repository branch Feb 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment