General fixes for 2.0 release #131

Merged
merged 20 commits into from Feb 16, 2017

Projects

None yet

3 participants

@webimpress

Please see commits descriptions for more details

@@ -100,13 +105,11 @@ public function testErrorHandler(
public function errorHandlerProvider()
{
- // @codingStandardsIgnoreStart
// $installType, $containerOption, $errorHandlerOption, $copyFilesKey, $expectedErrorHandler
@xtreamwayz
xtreamwayz Feb 15, 2017 Member

Remove $copyFilesKey from the comment as well.

@webimpress webimpress Removed redundant comment
9f53457
+ if ($expectedRoute['name'] === $route->getName()) {
+ $this->assertEquals($expectedRoute['path'], $route->getPath());
+ // todo: not sure how we can test middleware?
+ // $this->assertEquals($expectedRoute['middleware'], $route->getMiddleware());
@webimpress
webimpress Feb 15, 2017 edited

Any ideas how we can do that? In a nice way ...

@weierophinney
weierophinney Feb 15, 2017 Member

We need to do the following:

  • Split the logic currently in ProjectSandboxTrait into two separate methods:
    • one for setting up the application, pipeline, and routes
    • one for dispatching the provided $path
  • Update all calls in the test suite to getAppResponse() to instead call the above two methods in succession.
  • In this particular method, we'd call the method for setup first, and then loop over the routes, passing the path for each to getAppResponse().
    • For the "minimal" cases, we'd need to have a way of providing the routes without injecting them so we can explicitly test the 404 cases.
@webimpress
webimpress Feb 16, 2017

How do we know then if correct middleware is executed? As I understand correctly, with your suggested changes we will check just response codes for each route.
It's hard to check what middleware is bind for specific route, what I wanted to check here.
Maybe it's not important and what you've suggested will be sufficient?

@webimpress
webimpress Feb 16, 2017

Not sure also if splitting getAppResponse method into two methods is good, because then we kinda lost isolation for requests. In general, it is possible that the previous request can make some changes and then the next one can be successful or not (because of changes made by previous call - not sure if it is clear, but in real world we are going to have one request per one application build). We assume, the state of the $app is not changed after the request, and it's the same as before. I would then keep this method as it is, still we can test all routes and responses, but still it's not gonna tell us, if correct (route) middlewares has been dispatched.

@xtreamwayz
xtreamwayz Feb 16, 2017 Member

How do we know then if correct middleware is executed?

You check the response code so you know it executed ok. And you can check the response body for specific strings to make sure the correct action is executed. I think it doesn't matter what exactly is executed as long as you get the right code and content. To make sure the right content is generated in an action you need to test each action separately.

With those checks, I think you can be sure it works as intended.

@webimpress
webimpress Feb 16, 2017

Do we need it here then? If so I have to add something more to property expectedRoutes what should be expected response (code and content).

@weierophinney
weierophinney Feb 16, 2017 Member

The purpose of this test is to ensure that the routes are registered, and that the application can route to them correctly, which is why we only look at status codes. For the non-minimal setups, a 404 would indicate a problem with the configuration; for the minimal setup, a 200 would indicate a problem with the configuration.

I do see your point about not splitting up getAppResponse(), and would be fine with keeping it as-is, and simply calling it from within the foreach loop.

@webimpress
webimpress Feb 16, 2017

Not sure if it gives us more confident, because:

  1. we already know that this routing is available (we get routes from app $app->getRoutes())
  2. container is shared, and if we are not gonna split up getAppResponse we need to use clear container for every request, and... as container is shared and we get Application from container it's the same instance of Application.

I would then suggest to remove checks for middleware, and available routes with expected should be sufficient. Thoughts?

@weierophinney

Provided some ideas on how to proceed with the Routers tests.

+ if ($expectedRoute['name'] === $route->getName()) {
+ $this->assertEquals($expectedRoute['path'], $route->getPath());
+ // todo: not sure how we can test middleware?
+ // $this->assertEquals($expectedRoute['middleware'], $route->getMiddleware());
@weierophinney
weierophinney Feb 15, 2017 Member

We need to do the following:

  • Split the logic currently in ProjectSandboxTrait into two separate methods:
    • one for setting up the application, pipeline, and routes
    • one for dispatching the provided $path
  • Update all calls in the test suite to getAppResponse() to instead call the above two methods in succession.
  • In this particular method, we'd call the method for setup first, and then loop over the routes, passing the path for each to getAppResponse().
    • For the "minimal" cases, we'd need to have a way of providing the routes without injecting them so we can explicitly test the 404 cases.
@@ -17,17 +17,17 @@ class SetupDataAndCacheDirTest extends OptionalPackagesTestCase
/**
* @var OptionalPackages
*/
- protected $installer;
+ private $installer;
@weierophinney
weierophinney Feb 15, 2017 Member

Why are you changing all of these to private? Is there a compelling reason? (I'm not convinced visibility matters much in test cases, to be honest.)

@webimpress
webimpress Feb 16, 2017

It doesn't matter. I've changed it just for consistence. Everywhere in expressive tests we have private properties in test classes.

@weierophinney
weierophinney Feb 16, 2017 Member

Consistency is good, then.

webimpress added some commits Feb 16, 2017
@webimpress webimpress Update travis configuration
- renamed variables for consistency
- moved notification part to the end (consistency)
- simplify conditions
- added COVERAGE_DEPS variable
034487f
@webimpress webimpress Removed todo comment
- we don't need to check middleware in this test
This test already check one request - "home" and status code response.
We already know that these routings are binded to the application.
f3562f5
@weierophinney

Changes are good!

@weierophinney weierophinney merged commit f3562f5 into zendframework:develop Feb 16, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 96.237%
Details
@weierophinney weierophinney added a commit that referenced this pull request Feb 16, 2017
@weierophinney weierophinney Merge branch 'feature/131' into develop
Close #131
6df9719
@webimpress webimpress deleted the webimpress:fixes branch Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment