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.

Sign up for GitHub

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

Jump to bottom

API only config #355

Closed
wants to merge 2 commits into from
Closed

API only config #355

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
adbd697
feat: api only flag
hungthai1401 Apr 3, 2024
413c487
feat: change way to load base test case
hungthai1401 Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions config/log-viewer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

'enabled' => env('LOG_VIEWER_ENABLED', true),

'api_only' => env('LOG_VIEWER_API_ONLY', false),

'require_auth_in_production' => true,

/*
Expand Down
4 changes: 4 additions & 0 deletions src/LogViewerServiceProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ protected function registerRoutes()
$this->loadRoutesFrom(self::basePath('/routes/api.php'));
});

if (config('log-viewer.api_only', false)) {
return;
}

Route::group([
'domain' => config('log-viewer.route_domain', null),
'prefix' => config('log-viewer.route_path'),
Expand Down
18 changes: 18 additions & 0 deletions tests/Feature/RoutesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
<?php

use Symfony\Component\Routing\Exception\RouteNotFoundException;

test('the default url can be changed', function () {
config()->set('log-viewer.route_path', 'new-log-route');

Expand All @@ -25,6 +27,22 @@
expect(route('log-viewer.index'))->toBe('http://localhost');
});

test('only use api', function () {
config()->set('log-viewer.api_only', true);

reloadRoutes();

route('log-viewer.index');
})->throws(RouteNotFoundException::class);

test('only both api and web', function () {
config()->set('log-viewer.api_only', false);

reloadRoutes();

expect(route('log-viewer.index'))->toBe('http://localhost');
});

/*
|--------------------------------------------------------------------------
| HELPERS
Expand Down
1 change: 1 addition & 0 deletions tests/Pest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this line should still make the tests pass, unless you have locally cached the config (which you shouldn't).

Copy link
Author

Choose a reason for hiding this comment

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

@arukompas If remove this line, I can't pass my rule while it didn't receive new config

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

@hungthai1401 hungthai1401 Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@arukompas I have updated Pest.php when loading base test case to ignore my test case. Plz check my latest commit. Thanks

uses()->beforeEach(function () {
// let's not include any of the default mac logs or similar
config(['log-viewer.include_files' => ['*.log', '**/*.log']]);
Expand Down
Loading