Skip to content

Feature: Add Tests for Scrape Action#177

Merged
128na merged 10 commits into
masterfrom
feature/test-scrape
Jun 2, 2026
Merged

Feature: Add Tests for Scrape Action#177
128na merged 10 commits into
masterfrom
feature/test-scrape

Conversation

@128na

@128na 128na commented Jun 2, 2026

Copy link
Copy Markdown
Owner

This PR adds tests for the Scrape actions in SimutransCrossSearch.

Changes:

  • Added tests/Feature/Actions/Scrape/FetchHtmlTest.php to verify HTTP fetching and caching logic.
  • Added tests/Feature/Actions/Scrape/ScrapeActionTest.php to verify that all handlers are invoked correctly.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces feature tests for the FetchHtml and ScrapeAction classes. While FetchHtmlTest successfully covers HTTP requests, caching, and encoding conversion, the tests in ScrapeActionTest rely on dummy assertions (assertTrue(true)) that only verify no exceptions are thrown. It is recommended to mock HandlerFactory and HandlerInterface to properly assert that the correct handlers are resolved and invoked.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +7 to +38
use App\Actions\Scrape\ScrapeAction;
use App\Enums\SiteName;
use Illuminate\Support\Facades\Http;
use Psr\Log\NullLogger;
use Tests\Feature\TestCase;

final class ScrapeActionTest extends TestCase
{
public function test_invokes_handlers_for_all_sites_when_null_provided(): void
{
Http::fake([
'*' => Http::response('<html><body></body></html>', 200),
]);

$scrapeAction = app(ScrapeAction::class);
$scrapeAction(null, new NullLogger);

// Just ensure no exceptions were thrown
$this->assertTrue(true);
}

public function test_invokes_specific_handler_when_site_provided(): void
{
Http::fake([
'*' => Http::response('<html><body></body></html>', 200),
]);

$scrapeAction = app(ScrapeAction::class);
$scrapeAction(SiteName::Japan, new NullLogger);

$this->assertTrue(true);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The current tests use a dummy assertion ($this->assertTrue(true)) and only verify that no exceptions are thrown. This is an assertionless test pattern that does not actually verify the behavior of ScrapeAction (i.e., that it resolves the correct handlers from HandlerFactory and executes them).\n\nBy mocking HandlerFactory and HandlerInterface, we can assert that:\n1. The factory is called with the expected site names.\n2. The resolved handlers are actually invoked.\n\nThis also eliminates the need for Http::fake() in this test, making it a true unit/feature test for ScrapeAction in isolation.

use App\Actions\Scrape\HandlerFactory;\nuse App\Actions\Scrape\HandlerInterface;\nuse App\Actions\Scrape\ScrapeAction;\nuse App\Enums\SiteName;\nuse Psr\Log\NullLogger;\nuse Tests\Feature\TestCase;\n\nfinal class ScrapeActionTest extends TestCase\n{\n    public function test_invokes_handlers_for_all_sites_when_null_provided(): void\n    {\n        $handler = $this->mock(HandlerInterface::class, function ($mock) {\n            $mock->shouldReceive('__invoke')->once();\n        });\n\n        $this->mock(HandlerFactory::class, function ($mock) use ($handler) {\n            $mock->shouldReceive('create')\n                ->once()\n                ->with(SiteName::cases())\n                ->andReturn([$handler]);\n        });\n\n        $action = app(ScrapeAction::class);\n        $action(null, new NullLogger);\n    }\n\n    public function test_invokes_specific_handler_when_site_provided(): void\n    {\n        $handler = $this->mock(HandlerInterface::class, function ($mock) {\n            $mock->shouldReceive('__invoke')->once();\n        });\n\n        $this->mock(HandlerFactory::class, function ($mock) use ($handler) {\n            $mock->shouldReceive('create')\n                ->once()\n                ->with([SiteName::Japan])\n                ->andReturn([$handler]);\n        });\n\n        $action = app(ScrapeAction::class);\n        $action(SiteName::Japan, new NullLogger);\n    }

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a52e2dbd30

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

]);

$action = app(ScrapeAction::class);
$action(null, new NullLogger);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid exercising the real portal handler in this test

When siteName is null, ScrapeAction expands to every SiteName, so this call also constructs the Portal handler and runs Portal\AllUrl, which queries App\Models\Portal\Article on the portal database connection. In a normal PHPUnit run that only prepares the application test database (and may not have the external portal schema/connection), this makes the new test fail with a database error even though HTTP is faked; mock the handler factory or otherwise isolate the action from the real Portal model query.

Useful? React with 👍 / 👎.

@128na

128na commented Jun 2, 2026

Copy link
Copy Markdown
Owner Author

ご指摘ありがとうございます! \Handler\ や \HandlerFactory\ が \ inal\ クラスであるためMockeryでの直接のモックが難しいことから、依存関係をそのまま解決させつつ、 \Http::assertSent()\ を使って各サイトのハンドラーが実際に正しいURLへリクエストを送信したかを検証する、より実用的な結合テストに修正しました。

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@128na

128na commented Jun 2, 2026

Copy link
Copy Markdown
Owner Author

ご指摘ありがとうございます! \Handler\ や \HandlerFactory\ が \ inal\ クラスであるためMockeryでの直接のモックが難しいことから、依存関係をそのまま解決させつつ、 \Http::assertSent()\ を使って各サイトのハンドラーが実際に正しいURLへリクエストを送信したかを検証する、より実用的な結合テストに修正しました。

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@128na 128na merged commit 861e629 into master Jun 2, 2026
5 checks passed
@128na 128na deleted the feature/test-scrape branch June 2, 2026 13:35
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