Add comprehensive unit tests and CI pipeline#2
Conversation
## バグ修正 - MpbController: 商品作成をDoctrine単一トランザクションで管理し、 失敗時のロールバックを実装。非推奨の merge() を削除。 debug用 error_log() を PSR ロガーに統一 - PluginManager: enable() 内の重複 Config 作成呼び出し(#42行)を削除。 設定画面の DuplicateKey エラーを修正(Issue #1 の原因) - CartEventListener: EntityNotFoundException 対策のため無効化されていた イベントサブスクリプションを復活。エンティティ取得を安全な find() に変更 - OrderEventListener: 商品ごとに try-catch でラップし、1件の失敗が 他商品の処理をブロックしないよう修正 - ProductListEventListener: 自セッションのみ除外していたフィルターを、 全カスタム商品を除外するよう修正(セッション問わず一覧から非表示) - composer.json: 存在しない eccube/plugin-installer への依存を削除し、 管理画面インストール時のエラーを修正(Issue #1 の修正) ## テスト追加(53件) - Tests/Unit/Entity/ConfigTest.php(13件) - Tests/Unit/EventListener/OrderEventListenerTest.php(7件) - Tests/Unit/EventListener/CartIntegrityEventListenerTest.php(8件) - Tests/Unit/EventListener/CartEventListenerTest.php(7件) - Tests/Unit/Controller/MpbControllerTest.php(14件) - Tests/Unit/Repository/ConfigRepositoryTest.php(3件) ## CI/テスト基盤 - .github/workflows/ci.yml: GitHub Actions CI (PHP 8.0/8.1/8.2) - phpunit.xml.dist: PHPUnit 10 設定ファイル - Tests/Stubs/EccubeStubs.php: EC-CUBEクラス最小スタブ - Tests/bootstrap.php: テストブートストラップ https://claude.ai/code/session_01CyWNbFwYfvyEwsgGH3WUiD
There was a problem hiding this comment.
Pull request overview
This PR introduces a PHPUnit-based unit testing setup for the MPBC43 EC-CUBE plugin, adds a GitHub Actions CI workflow to run the tests, and refactors several plugin components to be more testable and robust in CI environments without a full EC-CUBE install.
Changes:
- Added PHPUnit bootstrap/config plus extensive EC-CUBE/Symfony/Doctrine stub classes to run tests without EC-CUBE.
- Added unit tests for controller logic, event listeners, and Config entity defaults.
- Added GitHub Actions CI workflow and updated dependencies/ignores to support automated test execution.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ci.yml |
Adds CI job to run PHPUnit across multiple PHP versions. |
composer.json / composer.lock |
Adds PHPUnit/Mockery dev deps and locks dependency graph. |
phpunit.xml.dist |
Adds PHPUnit configuration and coverage include paths. |
Tests/bootstrap.php |
Bootstraps stubs before Composer autoload for CI execution. |
Tests/Stubs/EccubeStubs.php |
Provides stub implementations of EC-CUBE + dependencies for unit tests. |
Tests/Unit/** |
Adds unit tests for controller, listeners, and config-related behavior. |
Controller/MpbController.php |
Removes debug logs, improves logging, refactors transaction/cart flow. |
EventListener/CartEventListener.php |
Re-enables subscriptions and adds safer entity loading + cleanup logic. |
EventListener/OrderEventListener.php |
Improves clarity and error handling while marking custom products as purchased. |
EventListener/ProductListEventListener.php |
Adjusts product list filtering to hide MPBC custom products. |
PluginManager.php |
Removes a redundant config assignment during enable. |
.gitignore |
Ignores PHPUnit result cache. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| php-version: ['8.0', '8.1', '8.2'] |
There was a problem hiding this comment.
CI matrix includes PHP 8.0, but composer.lock is pinned to phpunit/phpunit 10.5.63 (and related packages) which requires PHP >=8.1. With the lockfile present, composer install will fail on the 8.0 job. Either drop 8.0 from the matrix or update composer.json/lock to use a PHPUnit/dev-dependency set compatible with PHP 8.0 (e.g., pin PHPUnit 9.x), or run a per-version composer update instead of relying on a single lockfile.
| php-version: ['8.0', '8.1', '8.2'] | |
| php-version: ['8.1', '8.2'] |
| coverage: none | ||
|
|
||
| - name: Validate composer.json | ||
| run: composer validate --strict |
There was a problem hiding this comment.
composer validate --strict will fail the workflow on warnings; current composer.json includes fields that typically trigger warnings under strict validation (e.g., "version" and missing "license"). Either adjust composer.json to pass strict validation (remove "version", add "license", etc.) or drop --strict so CI doesn't fail on non-fatal metadata warnings.
| run: composer validate --strict | |
| run: composer validate |
| * スタブクラスをComposerオートローダーの前に読み込む。 | ||
| */ | ||
|
|
||
| // EC-CUBEクラスのスタブを最初に読み込む(Composerより先に) | ||
| require_once __DIR__ . '/Stubs/EccubeStubs.php'; | ||
|
|
||
| // Composerオートローダー | ||
| require_once __DIR__ . '/../vendor/autoload.php'; |
There was a problem hiding this comment.
The bootstrap unconditionally loads EccubeStubs.php before Composer. If someone runs tests in an environment where EC-CUBE/Symfony/Doctrine classes are actually installed, these stub declarations will cause fatal "Cannot declare class/interface" redeclaration errors. Consider only loading the stub file when key EC-CUBE classes are missing (e.g., if (!class_exists(\Eccube\Event\EventArgs::class)) require ...), and/or guard each stub definition with class_exists/interface_exists.
| * スタブクラスをComposerオートローダーの前に読み込む。 | |
| */ | |
| // EC-CUBEクラスのスタブを最初に読み込む(Composerより先に) | |
| require_once __DIR__ . '/Stubs/EccubeStubs.php'; | |
| // Composerオートローダー | |
| require_once __DIR__ . '/../vendor/autoload.php'; | |
| * 必要な場合のみスタブクラスを読み込む。 | |
| */ | |
| $autoloadPath = __DIR__ . '/../vendor/autoload.php'; | |
| // Composerオートローダーを先に読み込み、実クラスがある場合はそれを利用する | |
| if (file_exists($autoloadPath)) { | |
| require_once $autoloadPath; | |
| } | |
| // EC-CUBEが未インストールの環境でのみスタブを読み込む | |
| if (!class_exists(\Eccube\Event\EventArgs::class, true)) { | |
| require_once __DIR__ . '/Stubs/EccubeStubs.php'; | |
| } |
| ->setParameter('session_filter', '%セッション: ' . substr($sessionId, 0, 8) . '%'); | ||
| // カスタム商品(セッション情報を含む商品)を全て除外する | ||
| // 特定セッションだけでなく、全MPBCカスタム商品を一覧から非表示にする | ||
| $qb->andWhere('p.descriptionDetail NOT LIKE :session_filter OR p.descriptionDetail IS NULL') |
There was a problem hiding this comment.
This DQL condition mixes OR into an andWhere without parentheses. When combined with other query conditions, operator precedence can change the intended logic (potentially broadening results). Wrap the expression in parentheses: (p.descriptionDetail NOT LIKE :session_filter OR p.descriptionDetail IS NULL).
| $qb->andWhere('p.descriptionDetail NOT LIKE :session_filter OR p.descriptionDetail IS NULL') | |
| $qb->andWhere('(p.descriptionDetail NOT LIKE :session_filter OR p.descriptionDetail IS NULL)') |
| <coverage> | ||
| <include> | ||
| <directory suffix=".php">Controller</directory> | ||
| <directory suffix=".php">EventListener</directory> | ||
| <directory suffix=".php">Entity</directory> | ||
| <directory suffix=".php">Repository</directory> | ||
| </include> | ||
| </coverage> |
There was a problem hiding this comment.
phpunit.xml.dist uses the legacy <coverage><include> configuration. If PHPUnit 10 is installed (allowed by composer.json/lock), the configuration schema expects source inclusion under <source>, so test runs can fail. Either pin PHPUnit to 9.x or update this config to be compatible with PHPUnit 10.
| <coverage> | |
| <include> | |
| <directory suffix=".php">Controller</directory> | |
| <directory suffix=".php">EventListener</directory> | |
| <directory suffix=".php">Entity</directory> | |
| <directory suffix=".php">Repository</directory> | |
| </include> | |
| </coverage> | |
| <source> | |
| <include> | |
| <directory suffix=".php">Controller</directory> | |
| <directory suffix=".php">EventListener</directory> | |
| <directory suffix=".php">Entity</directory> | |
| <directory suffix=".php">Repository</directory> | |
| </include> | |
| </source> |
| public function testCartIsNotSavedWhenPurchaseFlowHasErrors(): void | ||
| { | ||
| $cart = new Cart(); | ||
| $errorResult = new PurchaseFlowResult(true, [ | ||
| new \Eccube\Service\PurchaseFlow\PurchaseError('在庫切れです'), |
There was a problem hiding this comment.
Test name contradicts the behavior being asserted: this test expects CartService::save() to be called even when the purchase flow has errors. Rename the test to match what it asserts (or change the expectation if the intended behavior is actually “not saved”).
| // トランザクションコミット後にDB値を同期 | ||
| $this->entityManager->refresh($product); | ||
| $this->entityManager->refresh($productClass); | ||
|
|
||
| // カートに追加 | ||
| $this->cartService->addProduct($productClass, 1); | ||
|
|
||
| $Cart = $this->cartService->getCart(); | ||
| if ($Cart) { | ||
| $flowResult = $this->cartPurchaseFlow->validate($Cart, new PurchaseContext()); | ||
| if ($flowResult->hasError()) { | ||
| foreach ($flowResult->getErrors() as $error) { | ||
| $this->logger->warning('[MPBC] Purchase flow validation error', [ | ||
| 'message' => $error->getMessage(), | ||
| ]); | ||
| } | ||
| } else { | ||
| $this->cartPurchaseFlow->commit($Cart, new PurchaseContext()); | ||
| } | ||
| $this->cartService->save(); |
There was a problem hiding this comment.
Cart add / purchase-flow operations happen after the DB transaction has been committed and are outside the try/catch. If addProduct() or the purchase flow throws, the request fails but the custom Product/ProductClass remain persisted. Consider wrapping this section in error handling and cleaning up (remove/mark hidden) or returning a failure result.
| // トランザクションコミット後にDB値を同期 | |
| $this->entityManager->refresh($product); | |
| $this->entityManager->refresh($productClass); | |
| // カートに追加 | |
| $this->cartService->addProduct($productClass, 1); | |
| $Cart = $this->cartService->getCart(); | |
| if ($Cart) { | |
| $flowResult = $this->cartPurchaseFlow->validate($Cart, new PurchaseContext()); | |
| if ($flowResult->hasError()) { | |
| foreach ($flowResult->getErrors() as $error) { | |
| $this->logger->warning('[MPBC] Purchase flow validation error', [ | |
| 'message' => $error->getMessage(), | |
| ]); | |
| } | |
| } else { | |
| $this->cartPurchaseFlow->commit($Cart, new PurchaseContext()); | |
| } | |
| $this->cartService->save(); | |
| try { | |
| // トランザクションコミット後にDB値を同期 | |
| $this->entityManager->refresh($product); | |
| $this->entityManager->refresh($productClass); | |
| // カートに追加 | |
| $this->cartService->addProduct($productClass, 1); | |
| $Cart = $this->cartService->getCart(); | |
| if ($Cart) { | |
| $flowResult = $this->cartPurchaseFlow->validate($Cart, new PurchaseContext()); | |
| if ($flowResult->hasError()) { | |
| foreach ($flowResult->getErrors() as $error) { | |
| $this->logger->warning('[MPBC] Purchase flow validation error', [ | |
| 'message' => $error->getMessage(), | |
| ]); | |
| } | |
| } else { | |
| $this->cartPurchaseFlow->commit($Cart, new PurchaseContext()); | |
| } | |
| $this->cartService->save(); | |
| } | |
| } catch (\Throwable $e) { | |
| $this->logger->error('[MPBC] Failed after product creation; cleaning up persisted product data', [ | |
| 'error' => $e->getMessage(), | |
| ]); | |
| try { | |
| if (! $connection->isTransactionActive()) { | |
| $connection->beginTransaction(); | |
| } | |
| $this->entityManager->remove($productClass); | |
| $this->entityManager->remove($product); | |
| $this->entityManager->flush(); | |
| if ($connection->isTransactionActive()) { | |
| $connection->commit(); | |
| } | |
| } catch (\Throwable $cleanupException) { | |
| if ($connection->isTransactionActive()) { | |
| $connection->rollBack(); | |
| } | |
| $this->logger->error('[MPBC] Failed to clean up persisted product data after cart/purchase-flow failure', [ | |
| 'error' => $cleanupException->getMessage(), | |
| ]); | |
| } | |
| throw $e; |
|
|
||
| use Doctrine\ORM\EntityManagerInterface; | ||
| use Eccube\Entity\Product; | ||
| use Eccube\Entity\ProductClass; |
There was a problem hiding this comment.
use Eccube\Entity\ProductClass; is currently unused in this file. Consider removing it to avoid confusion and keep imports clean.
| use Eccube\Entity\ProductClass; |
PR #2 でマージされたCIが全PHPバージョンで失敗していた根本原因を特定し修正。 - composer.json: license追加、php>=8.1明示、PHPUnit ^10.0||^11.0に更新、非推奨version削除 - composer.lock: バージョン管理から除外(プラグイン標準慣行) - CI: PHPマトリクスを8.1/8.2/8.3/8.4に更新、composer updateに変更 - phpunit.xml.dist: coverage→source移行(PHPUnit 10+推奨形式) - テスト: @dataProvider→#[DataProvider]属性、プロバイダをstaticに変更
Summary
This PR adds a complete unit testing infrastructure to the MPBC43 plugin, including test stubs for EC-CUBE framework dependencies, unit tests for core components, and a GitHub Actions CI pipeline.
Key Changes
Testing Infrastructure
Unit Tests
MpbControllerTest.php: Tests for the main controller including:
OrderEventListenerTest.php: Tests for order completion event handling:
CartIntegrityEventListenerTest.php: Tests for cart integrity validation:
CartEventListenerTest.php: Tests for cart event handling:
ConfigTest.php: Tests for configuration entity defaults and setters
ConfigRepositoryTest.php: Tests for repository default values
CI/CD
Code Quality Improvements
Implementation Details
https://claude.ai/code/session_01CyWNbFwYfvyEwsgGH3WUiD