Added spoken languages for movie hydration.

[mihaeu]

Spoken languages were pulled from the API, but not added to the movie object by the factory.

[mihaeu]

I would like to verify that it works with a test, but the mocked repo didn't allow me to load a movie. There are no assertions for those tests, which produces nice, but misleading coverage. What do I need to change so that I can mock a response object? Would that be the factory?

[wtfzdotnet]

First off all forgive me for the probably unclear way of things are tested, and probably not everything as it should be but my experience with testing has been fairly minimal.

Repository tests only verify the fact the request should have been send through guzzle, however what you want to achieve should be done through https://github.com/wtfzdotnet/php-tmdb-api/blob/develop/test/Tmdb/Tests/Factory/MovieFactoryTest.php#L71 it loads up this response saved from the API: https://github.com/wtfzdotnet/php-tmdb-api/blob/develop/test/Tmdb/Tests/Resources/movie/all.json

[wtfzdotnet]

Should call setSpokenLanguages()

[mihaeu]

Ups, made the correct change directly for my app and then copy pasted the wrong one. Sorry for that. I'll fix it.

OT:
Regarding the testing, maybe I'll add some tests for the API stuff later (if you want). It'd be nice to check for changes in the API automatically from the test suite. The way it is now the tests will pass even if the API keys change or anything like that (which would break main functionality). I'll open up an issue later (maybe having one fast and another slow testsuite which makes actual requests [at least maybe one per repo] would be an option).

[wtfzdotnet]

@mihaeu If you would like to take that upon yourself, please do 👍 :-). ( I'm actually screaming yes through the roof ;) ). Would be awesome to see how you propose to change the unit tests and why, eager to learn :-).

I actually intended not to make the API calls to keep the unit tests fast, although I understand your concerns if something within their API changes which breaks things this won't be handled properly.

I'd even be happy to help collaborating on these improvements together, just let me know.

[wtfzdotnet]

@mihaeu do you still intend to include the tests for this in the current state?

[mihaeu]

What do you think about opening another issue for that? The tests are not just for the language thing, but more about the load() of the repositories.

The problems I see is that on the one hand tests without an assertion are not a tests and on the other hand that end-to-end tests are expensive (but in the case of this API I think they are necessary).

[wtfzdotnet]

Yes of course that should be in an separate issue, I was only referring to the assertion directly coupled to the spoken languages.

[mihaeu]

Okay so if I would go about validating the functionality that was broken/missing before I could use the all.json from the test resources.

I went digging in the test setup and found that you mocked the send method of Guzzle.

        $httpClient = $this->getMock('Guzzle\Http\Client', array('send'));
        $httpClient
            ->expects($this->any())
            ->method('send')
            ->will($this->returnValue($response))
        ;

so if I want to get the fake result I should also mock the get() method and make it return the all.json instead, right?

That would keep the tests fast, but would allow for real assertions.

    public function get($path, array $parameters = array(), $headers = array())
    {
        /**
         * @var Response $response
         */
        $response = $this->client->getHttpClient()->get($path, $parameters, $headers);

        return $response->json();
    }

Later a cache layer could be used for real end-to-end tests, where every repository gets to make one real call which is cached and that would verify the API is still fully functional. If that would still be too slow I was thinking that limiting these tests to Travis would also be good enough, because the API shouldn't actually brake all that much and Travis will notify immediately, but tests run much faster on their servers.

Something like

if (true !== getenv('CI') || true !== getenv('TRAVIS')) {
    $this->markTestSkipped('Test should only run on Travis CI.');
}

What do you think? Sorry if I got you wrong. It's still early and I need another coffee.

[wtfzdotnet]

I don't believe that's the case here, if you take a look at the HttpClient\HttpClient class;

    public function get($path, array $parameters = array(), array $headers = array())
    {
        $parameters = $this->buildQueryParameters($parameters);

        return $this->request(
            $this->client->get($path, $headers, $parameters)
        );
    }

And the request method;

public function request(RequestInterface $request)
    {
        $response = null;

        try {
            $response = $request->send();
        } catch (ClientErrorResponseException $e) {
            $error = $e->getResponse()->json();
            throw new TmdbApiException($error['status_message'], $error['status_code']);
        }

        $this->lastRequest  = $request;
        $this->lastResponse = $response;

        return $response;
    }

You can see the last method responsible is the $request->send() method, the previous methods only generate the actual request objects and pass these on.

If I misunderstood your point please do let me know :-).

I'm not entirely sure though how much work would be involved making the actual requests and asserting on the actual responses. If I'm not mistaken you would like to change the tests so that the actual objects in the repository are asserted against their respective response earlier from the API?

[mihaeu]

I haven't forgotten about this one, just a bit busy/lazy these days. Could you merge this one as is? I know it's not tested (yet), but then the changes are pretty trivial and missing language info is a bummer for anyone using the API (or at least a big percentage).

[wtfzdotnet]

I'll just merge it in, and create a new ticket later for the whole unit testing discussion.