Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide basic integration testing with Request and Response objects. #41

Open
wants to merge 1 commit into
base: 3.3/develop
Choose a base branch
from

Conversation

arteymix
Copy link

This is a proposal for a skeleton TestCase that would be used to do integration testing with Kohana application.

I managed to test my application by self-requesting using internal Request just like the following:

$response = Request::factory('some/internal/endpoint')
    ->method(Request::POST)
    ->post('key', 'value')
    ->execute();

$this->assertEquals(200, $response->status(), $response->body());

But this gets a little messy and a lot of work has to be duplicated (especially for other status codes).

What I propose is a simple TestCase_Integration that would provide primitives for testing Response objects, making building and testing quality application simpler.

$this->assertOk($response);
$this->assertPermanentRedirect('http://some/location', $response);

I would like to have your insights on the matter!

@arteymix arteymix force-pushed the 3.3/feature/integration-testcase branch from 5167ef9 to 52e2f6e Compare November 25, 2014 18:41
@arteymix arteymix force-pushed the 3.3/feature/integration-testcase branch from 52e2f6e to e64bc5e Compare December 12, 2014 12:25
@arteymix
Copy link
Author

I squashed everything in a single commit and rename the TestCase to suit Database_TestCase naming.

{
if ($message === NULL)
{
$message = $response->body();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this potentially dump an enormous HTML string into your PHPUnit output? Don't think I like that idea.

@acoulton
Copy link
Member

Not sure what I think about this. I think some of the assertion names (eg assertOk) aren't very clear what they're actually asserting. I'd prefer a single assertHttpStatus(200, $response) when reading the tests, which would also reduce risk of name conflicts. You could use constants for the status codes to avoid having magic numbers dotted around the tests.

Obviously the redirect assertions are more useful, because they're verifying the full state of the redirection.

I think if we were going to distribute this, we'd need to unit test the assertions themselves, I'm not a fan of assertion methods that don't have coverage as it's hellish if an assertion doesn't actually test what you think it does.

I also don't know if it's a good idea to encourage people to do these sorts of tests in-process, because I don't think Kohana's global state is well-enough isolated to make the request execution independent. I'd generally want to test at this level over HTTP, either with Behat or for example by driving Browserkit or Mink through a PHPUnit test suite.

$this->assertStatus(200, $response, $message);
}

public function assertPermanentRedirection($location, Response $response, $message = NULL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be better as a first-class PHPUnit assertion, so that if it fails it can show the whole of the status/location header/etc in a useful failure message that describes the overall assertion and what was incorrect, rather than a chain of assertEquals.

For example, Expected 200 to equal 301 but it did not doesn't tell you whether the Location header was set or not, so you don't know if the issue is that the application never redirected, or it did but the status code went wrong along the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants