API only config #355
API only config #355
Conversation
tests/Pest.php
Outdated
| @@ -11,6 +11,7 @@ | |||
| uses(TestCase::class)->in(__DIR__); | |||
| uses()->afterEach(fn () => clearGeneratedLogFiles())->in('Feature', 'Unit'); | |||
| uses()->beforeEach(fn () => Artisan::call('log-viewer:publish'))->in('Feature'); | |||
| uses()->beforeEach(fn () => Artisan::call('config:cache'))->in('Feature'); | |||
There was a problem hiding this comment.
Really not sure about that. Why is it needed? Caching the config (or views, routes, etc) should only be done in production for the sake of performance improvement.
There was a problem hiding this comment.
Removing this line should still make the tests pass, unless you have locally cached the config (which you shouldn't).
There was a problem hiding this comment.
@arukompas If remove this line, I can't pass my rule while it didn't receive new config
There was a problem hiding this comment.
@hungthai1401 that is not the case. Not sure how config:cache helps here, but one of the tests might not be passing because calling reloadRoutes does not remove previously-loaded routes.
For example, when the test is first set up, the LogViewerServiceProvider::registerRoutes() is called already with whatever config it is at that point. If api_only is false, then it will register the web Log Viewer routes as well.
But then when you call reloadRoutes test helper later on, it calls LogViewerServiceProvider::registerRoutes() again, and at this point api_only is true, so it should skip registering the web routes for Log Viewer, right? And it skips it, but it makes no difference because the web routes have been registered already when setting up the test initially.
Need to find a way to update the reloadRoutes method so that it first unloads (unsets) previously set Log Viewer routes before calling LogViewerServiceProvider::registerRoutes() again.
There was a problem hiding this comment.
You can also see that config:cache doesn't really help when running the full test suite, as shown in the GitHub actions run on this PR.
There was a problem hiding this comment.
@arukompas I have updated Pest.php when loading base test case to ignore my test case. Plz check my latest commit. Thanks
ignore some test cases need register service provider laterly
|
hey @hungthai1401 I appreciate the help, but the latest change made it even more complex than before :) You were on the right track initially, just missing a way to "unload" previously-set routes so they can be registered again. Here's my attempt at this - #358, which includes the exact same tests and logic you did in the beginning, but with updated I'm happy for you to submit the same changes to get the contribution status, if you wish. Let me know what you think! |
|
@arukompas You saved my life. Your solution is so simple but smart. I will apply when your PR is merged. Thank you so much |
|
hey @hungthai1401 , tagged a new release, v3.8.0 which now contains this config option. Closing this PR as no longer needed. Thanks for idea and your work! 🙌 |
This PR introduces the api only config to avoid registering viewer route. Sometimes, I don't want to use the viewer so I had set its default value is
false.Usage:
Add
LOG_VIEWER_API_ONLY=trueto.env