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

Fix #2676 : add ability to test redirect()->route() via ControllerTester #2686

Merged
merged 8 commits into from
Mar 18, 2020

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Mar 11, 2020

Fixes #2676 to add ability to test redirect()->route() in test bootstrap so the existing route both in app and tests will be discovered.

Checklist:

  • Securely signed commits
  • Unit testing, with >80% coverage

@samsonasik
Copy link
Member Author

I've added unit test for it.

@samsonasik samsonasik changed the title Fix #2676 : add ability to test redirect()->route() via ControllerTester [wip] Fix #2676 : add ability to test redirect()->route() via ControllerTester Mar 11, 2020
@samsonasik
Copy link
Member Author

It seems this method only check routes in test/_support part, I will check to add namespace Config and App as well.

@samsonasik
Copy link
Member Author

samsonasik commented Mar 11, 2020

I got the solution for autoload in test, specifically the router, doesn't detect App and Config namespace in project structure in bootstrap test!

I register "App" and "Config" to admin/starter's composer.json autoload under psr-4 so when running test from the appstarter project in the next release, it will always work as it discoverable from composer as running test means developer using composer as autoloader.

@samsonasik samsonasik changed the title [wip] Fix #2676 : add ability to test redirect()->route() via ControllerTester Fix #2676 : add ability to test redirect()->route() via ControllerTester Mar 11, 2020
@samsonasik
Copy link
Member Author

travis build on php72:postgres is unrelated with it.

@samsonasik
Copy link
Member Author

I found a better solution. Instead of add the autoloader, I added a 1 line after setup routes, which is a require_once of APPPATH . 'Config/Routes.php', so the solution is add the following to system/Test/bootstrap.php:

$routes = \Config\Services::routes();
$routes->getRoutes('*');

require_once APPPATH . 'Config/Routes.php';

That's it! So developer don't need to update their composer.json.

@samsonasik
Copy link
Member Author

ready for review. travis build php73:mysqli is unrelated.

@samsonasik
Copy link
Member Author

actually, the APPPATH . 'Config/Routes.php already define Services::routes(true) and assign to $routes variable, so, I updated to use existing $routes variable and call getRoutes(*) on it:

require_once APPPATH . 'Config/Routes.php';
$routes->getRoutes('*');

@samsonasik
Copy link
Member Author

travis build green 🎉

@lonnieezell lonnieezell requested a review from MGatner March 12, 2020 03:18
@MGatner
Copy link
Member

MGatner commented Mar 12, 2020

This looks good and well written. I would like to test it with a couple existing projects and modules to make sure loading the routes that early doesn't effect other services, like mocking.

@samsonasik
Copy link
Member Author

@MGatner merge-able ?

@MGatner
Copy link
Member

MGatner commented Mar 18, 2020

I still haven't tried it! Let me do a couple right now.

@MGatner
Copy link
Member

MGatner commented Mar 18, 2020

Okay, looks good! Tested two projects and two modules and this change didn't seem to affect them.

@MGatner MGatner merged commit 84b9cc7 into codeigniter4:develop Mar 18, 2020
@MGatner
Copy link
Member

MGatner commented Mar 18, 2020

@samsonasik Thanks for the ping BTW. I've got a ton going on and the repo has been quite busy. If you see other things that merit my attention don't hesitate to mention me.

@samsonasik samsonasik deleted the fix-2676 branch March 18, 2020 19:09
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.

Bug: Can't test redirect()->route('routename'), while redirect()->to('path') is working with ControllerTester
2 participants