test: incomplete/skip で放置されたテストを棚卸しし、隠れていた実装の不具合を修正#6945
Open
nanasess wants to merge 16 commits into
Open
Conversation
以下の4箇所の markTestIncomplete を削除し、テストを有効化する。 いずれもマーカーを外すだけでアサーションが通ることを実測済み。 - Form/Type/Install/Step1TypeTest: setUp() のマーカー削除 (4 tests) - Service/EntityProxyServiceTest::testGenerateFromOriginalFile (4 assertions) - Service/Payment/PaymentMethodTest::testConstructorInjection (3 assertions) - Form/Type/Admin/OrderItemTypeTest::testInvalidQuantityNotNumeric Rector が markTestIncomplete を含むメソッドに付与していた戻り値型 `: never` も併せて除去している (残すと fatal error になるため)。 なお OrderItemTypeTest::testInvalidQuantityHasMinus は本コミットの 対象外とし、別途扱う。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
商品明細(PRODUCT)は OrderItemType の POST_SUBMIT リスナで 「金額 -> 正, 個数 -> 正負」が仕様であり、個数のマイナス値は valid となる。 このため markTestIncomplete で放置されていた既存テストの期待値は誤りだった。 個数の符号検証が実際に効く明細種別 (DISCOUNT / DELIVERY_FEE / CHARGE) を DataProvider で回し、個数がマイナスのとき invalid になることを検証する形に 書き換えた。DISCOUNT は金額 <= 0 が要求されるため、明細種別ごとに妥当な金額を 与えて個数のみを検証対象としている。 これまでどのテストからも通っていなかった DISCOUNT / DELIVERY_FEE / CHARGE の 符号検証分岐のカバレッジを埋める。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
参照先クラスが存在せず実行不能な、2017-2018 年の Silex 時代のテストを削除する。
- tests/Eccube/Tests/Command/CsvLoaderCommandTest.php
テスト対象の Eccube\Command\CsvLoaderCommand が存在しない (コマンド自体が廃止済み)。
CSV 読み込みエンジン Eccube\Doctrine\Common\CsvDataFixtures は現存するが、
tests/Eccube/Tests/Doctrine/Common/CsvDataFixtures/ の
CsvFixtureTest / LoaderTest / DbalExecutorTest が既にカバー済みのため代替テストは不要。
- tests/Eccube/Tests/Command/CommandTestCase.php
Knp\Command\Command / Knp\Provider\ConsoleServiceProvider /
Dbtlr\MigrationProvider\Provider\MigrationServiceProvider / Eccube\Application /
Eccube\Command\GeneratorCommand\AbstractPluginGenerator がすべて存在しない。
継承者は CsvLoaderCommandTest のみのため同時に削除。
- tests/Eccube/Tests/Transaction/TransactionListenerTest.php (ディレクトリごと)
Eccube\Application / Eccube\Tests\ServiceProvider\CsrfMockServiceProvider /
Eccube\Tests\ServiceProvider\FixtureServiceProvider が存在せず、$this->app[...] を 13 箇所使用。
テスト対象の src/Eccube/EventListener/TransactionListener.php は現存するため、
新規テストは後続コミットで作成する。
- tests/Eccube/Tests/Service/PluginServiceWithEntityExtensionTest.php
PHPUnit 5 時代の \PHPUnit_Framework_MockObject_MockObject を型宣言に使用しており実行不能。
- tests/Eccube/Tests/Web/Admin/Order/EditControllerWithMultipleTest.php
$this->app[...] を 13 箇所使用し、存在しない createShipping() ヘルパーを呼ぶ。
12 テスト中 8 つは EditControllerTest に生きた同名テストがあり重複。
固有の複数配送テストは後続コミットで新規作成する。
削除前に他ファイルからの参照が無いことを確認済み。
tests/Eccube/Tests/Web/Admin/Order/AbstractEditControllerTestCase.php は
EditControllerTest / ShippingControllerTest が使用するため残置。
検証:
- vendor/bin/phpunit tests/Eccube/Tests/Command/ tests/Eccube/Tests/Doctrine/ \
tests/Eccube/Tests/Service/PluginServiceTest.php
→ Tests: 80, Assertions: 169 (Failure/Error 0)
- vendor/bin/phpunit tests/Eccube/Tests/Web/Admin/Order/
→ Tests: 74, Assertions: 292 (Failure/Error 0)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2017 年から markTestIncomplete で死んでいた TransactionListenerTest を 実質的に復活させ, 無カバレッジだった TransactionListener を回帰検知下に置く。 DBAL 4 移行 (EC-CUBE#6747 / PR EC-CUBE#6855) で onKernelRequest の処理順が変わったため, 主目的として次の順序をモックの期待で固定する: 接続確立 (getNativeConnection) -> setAutoCommit(false) -> setTransactionIsolation -> beginTransaction 先に autoCommit=false にすると connect() 内で暗黙のトランザクションが開始され, 後続の beginTransaction がネストして onKernelTerminate の commit が効かず, 書き込みが永続化されない。この罠を順序アサーションで検知する。 その他, サブリクエストのスキップ, isEnabled=false / disable() 時の無処理, onKernelException / onKernelTerminate の rollback・commit 分岐, getSubscribedEvents の購読内容を検証する。 services_test.yaml で本 Listener は無効化されているためコンテナからは取得せず, モックを注入して手動でインスタンス化する。log_debug() が LoggerFacade に 依存するため EccubeTestCase を継承してカーネルを起動する。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
削除した Silex 由来の EditControllerWithMultipleTest のうち, 固有価値のあった
複数配送のカバレッジを現行の作法で書き直す。EditControllerTest には shipping /
Multiple への言及が無く, 管理画面 受注編集の複数配送は空白だった。
追加したシナリオ:
- testRoutingAdminOrderEditWithMultiple: 複数配送の受注で編集画面が表示できること
- testShippingAssociationIsPreservedOnEdit: Shipping 2 件の受注を明細を変更せず
mode=register で保存し, 商品明細の shipping_id 分布が保存前後で不変であること
(OrderType::associateOrderAndShipping が既存明細の Shipping を上書きすると
全商品が第1配送先へ集約される退行を検出する)
- testOrderEditWithShippingItemDelete: 複数配送で明細を削除しても他の配送に
紐づく明細が保持されること (旧 testOrderEditWithShippingItemDelete 相当)
フィクスチャの注意点:
- Generator::createOrder() は ProductClass を複数渡しても Shipping を 1 つしか
作らないため, 2 件目の Shipping と明細は手組みする
(ShippingRepositoryTest の前例に倣う)
- 手組みした明細には PurchaseFlow の TaxProcessor 相当の税率・税調整・丸め規則を
TaxRule から設定する。未設定だと Order::getTotalByTaxRate() が
RoundingType null で落ちる
- 複数配送時は OrderType::addShippingForm が Shipping フォームを追加しないため,
フォームデータから Shipping を除外する。OrderItems は
フォームへセットされる Order::getItems() (ソート済み) の順で組み立てる
感度確認: associateOrderAndShipping の未紐付けガードを外すと
testShippingAssociationIsPreservedOnEdit が
{1:1, 5:2} -> {1:1, 5:1} で落ちることを実測済み。
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CartService の lock()/unlock() は 4 系に存在せず (現存するのは clear() のみ)、
カートのロック概念自体が廃止されている。これらに依存したまま
markTestIncomplete でマークされていたテストを整理した。
削除:
- ShoppingControllerWithMultipleNonmemberTest::testAddMultiShippingCartUnlock
- ShoppingControllerWithMultipleNonmemberTest::testAddMultiShippingShippingUnlock
いずれも unlock() 前提で、現行仕様に対応する振る舞いが存在しないため削除。
EC-CUBE 3 系の $this->app['eccube.service.cart'] 参照が残っていた。
書き直し:
- ShoppingControllerWithMultiple{,Nonmember}Test::testAddMultiShippingWithoutCart
「カートが空の状態で複数配送設定画面へアクセスする」振る舞い自体は現行仕様でも
有効なため削除せず書き直した。ShippingMultipleController::index は
CartService::getPreOrderId() が null の場合 shopping_error へリダイレクトするため、
期待値を cart から shopping_error へ修正した。
CartService をテストプロセスから直接操作すると SessionNotFoundException となるため、
クライアントのセッションを介して cart_handle_item(remove) で明細を削除する方式にした。
併せて、4 系に存在しない /cart/add へのリクエスト (常に 404) を削除した。
- Admin/Order/EditControllerTest::testOrderProcessingToFrontConfirm
→ testOrderProcessingRegister へリネーム。
markTestIncomplete 以降のフロント購入確認フローが lock() 依存だったため削除し、
マーカー以前で実行済みだった「管理画面から PROCESSING で受注登録できる」検証を残した
(本ファイル唯一の PROCESSING カバレッジのため)。未使用となった CartService
プロパティと Constant の import も削除。
各ファイルの Failure/Error は 0、Incomplete も 0 になり、
EditControllerTest は 19 tests / 59 assertions とベースラインから
アサーション数が変わらないことを確認済み。
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
markTestIncomplete で無効化されたテストが、Silex 時代の API や廃止済みルート、 旧ヘルパーシグネチャを参照したまま放置されていたため、機械的に現行の作法へ揃える。 - CartValidationTest::scenarioComplete() の潜在デッドブランチを解消 ($confirmUrl 省略時に存在しない shopping_order を生成し RouteNotFoundException に なっていたため shopping_confirm へ変更) - 旧シグネチャ scenarioCartIn($client, $productClassId) / scenarioCartUp / scenarioCartDown / scenarioConfirm / scenarioComplete を、生きたテストと同じ (Customer $Customer, ProductClass $ProductClass, int $num) 形式へ修正 - Silex 残骸を除去 - $this->app['eccube.repository.base_info']->get() → $this->BaseInfo - $this->app['eccube.service.cart'] → static::getContainer()->get(CartService::class) - 現行 CartService に存在しない unlock() 呼び出しを削除 - $this->app->path() / $this->app->url() → $this->generateUrl() - MailCatcher 系ヘルパー → assertEmailCount() / getMailerMessage() - ShoppingControllerTest::testOrtderConfirmLayout の存在しない shopping_payment を shopping_redirect_to へ修正 - 未 import だった Eccube\Entity\Delivery を追加 マーカー解除は本コミットの範囲外。マーカーを外して実測した結果、残る失敗要因は 「販売種別違いはエラーではなくカート分割」という仕様変更と、購入フロー・マイページ履歴の UI 変更であり、いずれも次段階の対応となるため markTestIncomplete は維持し、 理由コメントのみ実態に合わせて更新した。 検証: 変更した 3 ファイルを個別に phpunit 実行し、いずれもベースライン (Failure/Error 0) を維持していることを確認。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
EC-CUBE 3 時代の「販売種別が異なる商品は同時購入を拒否する」仕様を期待していた 未完了テストを、4系の「販売種別ごとにカートを分割する」仕様へ書き換えて復活させた。 現行実装 (SaleTypeCartAllocator) は販売種別 ID をカートの識別子とするため、 販売種別違いの商品は購入を拒否されず決済単位ごとにカートが分割され、 「同時購入できない商品のカートを分けました。」が表示される。 復活させたテスト: - testProductInCartSaleType - testProductInCartSaleTypeWithMultiShipping - testProductInCartChangeSaleTypeBeforePlus - testProductInCartChangeSaleTypeBeforePlusWithMultiShipping - testProductInCartChangeSaleTypeBeforeMinus - testProductInCartChangeSaleTypeBeforeMinusWithMultiShipping 文言の差し替えに留めず、assertCartDivided で「カートが販売種別ごとに分割され、 各商品がちょうど 1 つのカートにのみ含まれる」ことを構造的に検証する。 分割が起きない変更を入れると 6 件すべてが落ちることを実測して確認済み。 マーカー理由「複数配送が実装されるまでスキップ」は陳腐化していた。複数配送は 実装済みであり、4系に複数配送の ON/OFF 設定 (BaseInfo) は存在しないため、 WithMultiShipping 系は「分割された各カートが独立したレジ導線 (cart_buystep) を 持つこと」の検証に置き換えた。 atestProductInCartSaleTypeFromShopping は名前に反して配送方法削除のテストであり、 atest プレフィクスで無効化されたままのため、誤ったマーカー理由のみ実態に修正した。 Tests: 50, Assertions: 146 (90 から増加), Incomplete: 14 → 8, Failure/Error 0 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
管理画面の商品検索は、旧テストが前提としていた「在庫なし」「公開」「非公開」の リンクを持たない。現行UIは検索フォームの stock / status セレクトで絞り込むため、 `selectLink()` はノードが空で InvalidArgumentException になっていた。 マーカー理由「検索項目(公開・非公開・在庫内)の実装完了後に実施」は陳腐化している。 SearchProductType には status (ProductStatusType) と stock (ChoiceType) が実装済みのため、 検索フォームを POST する形へ書き換えて markTestIncomplete を除去した。 - testExportWithFilterNoStock: stock=[OUT_OF_STOCK] で絞り込む形へ変更 - testExportWithFilterPrivate/Public: status=[DISPLAY_HIDE|DISPLAY_SHOW] で絞り込む形へ変更 - testExportWithAll: 到達しない2個目の markTestIncomplete (デッドコード) を除去し、 絞り込みなしで公開・非公開の両方が出力されることを検証する内容へ整理 - testProductExport: 期待どおり動作しない expectOutputRegex を除去 expectOutputRegex が機能しなかったのは、HttpKernelBrowser::filterResponse が StreamedResponse の sendContent() を自前の ob_start()/ob_end_clean() で包んでおり、 CSV が PHPUnit の出力バッファへ到達しないため (実測で content が '' になることを確認)。 出力の取得は getInternalResponse()->getContent() に統一し、検索と CSV 出力を searchProduct() / exportProductCsv() ヘルパへ集約した。 Rector が付けていた `: never` 戻り値型もマーカー除去に伴い外している。 Tests: 48, Assertions: 147, Deprecations: 2 (Failure/Error/Risky なし) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
マーカー理由「Impossible to call set("eccube.constants") on a frozen ParameterBag」は
今も正しい。EccubeConfig::offsetSet() は設計上 LogicException を投げる読み取り専用であり、
static::getContainer()->setParameter() も frozen ParameterBag には効かない。
つまり「設定を UTF-8 に書き換えて比較する」という手法自体が成立しない。
設定を書き換えるのではなく、期待値側を実際の出力エンコーディングへ合わせる形に改めた。
- testCsvTemplateWithProduct / testCsvTemplateWithCategory: 設定書き換えを廃止し、
出力を eccube_csv_export_encoding (既定 SJIS-win) から UTF-8 へ復号してから
列名の配列として比較する assertCsvTemplate() ヘルパへ集約
- 期待値をバイト列でなく列名配列にしたのは、SJIS は 2 バイト目に 0x5C
(fputcsv のエスケープ文字) を含む文字があり (「表」= 0x95 0x5C)、
その列だけ引用符で囲まれてバイト比較が壊れるため
- 商品雛形の期待値に、旧期待値が取りこぼしていた現行カラム
「商品規格表示フラグ」を追加
- Content-Type / Content-Disposition / StreamedResponse であることも併せて検証
- 出力の取得は前コミット (ProductControllerTest) と同じく
getInternalResponse()->getContent() に統一。HttpKernelBrowser::filterResponse が
StreamedResponse の sendContent() を自前の ob_start()/ob_end_clean() で包むため
expectOutputString では捕捉できない
- Rector が付けていた `: never` 戻り値型もマーカー除去に伴い外している
Tests: 44, Assertions: 223, Deprecations: 1 (Failure/Error/Risky/Incomplete なし)
Deprecation 1 件は vendor の mobiledetect 由来でベースラインから存在する。
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
markTestIncomplete で無効化されていた 7 件のテストを、実装を裏取りしたうえで 現行仕様に合わせて復活させた (Rector が付与していた `: never` 戻り値型も併せて除去)。 - Admin/Order/OrderControllerTest::testBulkOrderStatus 「使用していないルーティングのためスキップ」は事実誤認のため訂正。 admin_shipping_update_order_status は現存し、受注一覧の一括ステータス更新 (Order/index.twig の data-update-status-url) から実際に使われている。 真因は Generator が生成する Order が既定で PROCESSING であり NEW の受注が 0 件だったフィクスチャ不足。NEW の受注を 2 件用意するようにした。 併せて、更新後は対象ステータスへ遷移するため findBy の条件を遷移先に修正。 - ForgotControllerTest::testIndexWithPostAndVerify Symfony Mailer 移行の取り残し。本文取得を getBody() から getTextBody() に変更。 再設定画面の見出しを現行の訳語 (front.forgot.reset_title) に、 forgot_reset は reset_key 必須のためメール記載の URL へ POST し mypage_login へのリダイレクトを検証するよう修正。 - ContactControllerTest::testConfirm 確認ページへは遷移できており (Contact/confirm.twig が描画される)、 title のみ「入力ページ」になるのが実態。contact と contact_confirm は 同一パス /contact でルータは常に contact にマッチするため、 TwigInitializeListener が設定する title グローバルに コントローラの Page (contact_confirm) が反映されないことによる。 確認ページの描画と Page(noindex) の受け渡しを検証する内容に変更し、 title の制約は実装側の課題として NOTE を残した。 - Admin/Product/CategoryControllerTest::testMoveSortNoAndShow カテゴリツリーは各階層とも sort_no 降順で描画される (CategoryRepository::getList)。末尾は 子2-2 ではなく、最後の親 (親2) 配下の 最も深い末尾ノードである 孫2 が正。描画順そのものを検証するよう修正。 - Admin/Store/TemplateControllerTest::testDownload 実装の不具合ではなく BrowserKit 由来の問題。HttpKernelBrowser::doRequest() は handle() 直後に terminate() を呼び、その後 filterResponse() で sendContent() するため、kernel.terminate で一時ファイルを削除する download() では BinaryFileResponse の読み出し前にファイルが消える (実リクエストでは送信後に terminate されるため問題ない)。カーネルを直接 handle して検証し、 最後に terminate する形に変更した。 - ShoppingControllerTest::testPaymentEmpty 旧メッセージは現存しない。選択肢が空の場合は OrderType が Payment の NotBlank に front.shopping.payment_method_not_fount を設定する仕様のため、 確認画面では支払い方法が描画されないこと、確定時に当該エラーとなることを検証。 併せて未 flush だった支払い方法の利用条件変更を反映するよう修正。 - ShoppingControllerTest::testShippingShippingPost / testShippingEditTitle 旧 UI セレクタと既に存在しない MailCatcher ヘルパー前提だったため、 現行の shopping_shipping_edit フローに追従。電話番号は TruncateHyphenListener によりハイフンが除去される点も反映した。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
追加した未完了テスト復活分に対して Rector と PHP-CS-Fixer を適用した。
- データプロバイダの配列を Iterator (yield) へ変換
- assertSame(n, ->count()) を assertCount へ変換
- 不要な (string) キャストを除去
- find() の戻り値に assertInstanceOf を追加
- isset($phar['app']) を assertArrayHasKey へ変換
TransactionListenerTest では ContainerGetNameToTypeInTestsRector が
get('kernel') を get(Kernel::class) へ変換するが, 'kernel' はクラス名の
エイリアスを持たないため ServiceNotFoundException となり 13 件 error に
なった。rector.php に記載の方針どおり, ルールは無効化せずサービスIDを
変数へ代入して変換対象から外した。
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2017-2018 年から markTestIncomplete で放置されていた 2 件を解消する。 ValidatableItemProcessorTest::testValidateCartFail アサーションが 1 つも無く、マーカーを外しても risky になるだけだった。 ItemValidator::execute() には Cart/Order の分岐が無いため、対になる testValidateOrderFail と同じく handle() の呼び出しと ProcessResult が warning になることを検証する形に補完した。これで Success/Fail × Cart/Order の 4 通りが揃う。 PurchaseFlowTest::testAddProcesser 削除する。呼んでいた addItemHolderProcessor() / addItemProcessor() は addItemHolderPreprocessor() / addItemPreprocessor() に改名されており、 そのままでは fatal になる。ただし本体はこの 2 つを呼ぶだけで アサーションが無く、メソッド名を直しても「アサーション無しの重複 テスト」にしかならない。登録した Preprocessor が実際に効くことは testProcessItemProcessors / testProcessItemHolderProcessor が既に 検証済みのため、復活させる価値がない。 Rector が付与していた戻り値型 `: never` も併せて除去している。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Order::getTotalByTaxRate() は $roundingTypes[$rate]->getId() を null ガード なしで呼んでおり, RoundingType が未設定の課税明細が 1 件でもあると "Call to a member function getId() on null" になる。 OrderItem::getRoundingType() は ?RoundingType で null を返し得る。 対になる getTaxByTaxRate() には 2024-03-11 の f653af6 "Fix phpstan" で 同じガードが入っており, getTotalByTaxRate() だけが漏れていた。同じガードを 足して対称にする。 これは単なる防御ではなく実害の修正でもある。OrderPdfService は foreach ($Order->getTotalByTaxRate() as $rate => $total) { ... $Order->getTaxByTaxRate()[$rate] ... } と両者を突き合わせるため, 集計対象の税率がずれると未定義キーを引く。 本修正で両者の税率キーが常に一致する。 通常は PurchaseFlow の TaxProcessor が必ず RoundingType を設定するため 顕在化しにくいが, 明細を直接組み立てるカスタマイズ・プラグインからは到達し得る。 回帰テストは税率キーの集合が getTaxByTaxRate() と一致することを検証する (明細を足すと按分が変わるため金額の固定値では検証できない)。ガードを外すと "Call to a member function getId() on null" で落ちることを確認済み。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ContactController は contact と contact_confirm を同一パス '/contact' に
割り当てているため, ルータは常に先に定義された contact にマッチする。
TwigInitializeListener は _route から引いた Page 名を twig グローバル title に
設定するので, 確認ページでも title が「お問い合わせ(入力ページ)」になっていた。
コントローラが render に渡す Page(contact_confirm) は meta_tags には効いており
(noindex は正しく出力される), title だけが不整合な状態だった。
default_frame.twig の <title> は subtitle を title より優先するため, 確認ページの
render に subtitle として Page 名を渡して上書きする。ProductController が
'subtitle' => $Product->getName() と同じ手法を使っている前例に倣った。
ルーティングは変更しない。パスを分けると公開 URL が変わり後方互換性を壊すため。
ContactControllerTest::testConfirm に title の検証を追加した (これまで
markTestIncomplete('FIXME title') で放置されていたテスト)。subtitle を外すと
「EC-CUBE SHOP / お問い合わせ(入力ページ)」で落ちることを確認済み。
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OrderControllerTest::testSimpleUpdateOrderStatusWithSendMail Generator が生成する Order は既定で PROCESSING のため findBy(['OrderStatus' => NEW]) が 0 件を返し, foreach が回らず アサーションが 1 つも実行されないまま green になっていた。 先行コミットで testBulkOrderStatus に入れたのと同じ形で NEW の受注を 用意し, findBy の結果件数も検証する。 CsvImportControllerTest インポート処理がアップロードファイルを削除するため, fixture を __DIR__ にコピーしてから使っていた。このコピー先が git 管理下のため, テストを実行するだけで作業ツリーが汚れていた。コピー先を sys_get_temp_dir() に変更する (createCsvFromArray は元から一時 ディレクトリを使っており, それに揃える形)。 scenario() は $this->filepath を参照し, 'categories.csv' は UploadedFile の表示名にすぎないため, コピー先の変更による影響はない。 併せて tests/Eccube/Tests/Web/Admin/Product/categories.csv を削除する。 これは上記のコピー先に生成されたテスト成果物が 8740f32 で誤って コミットされたもので, コピー元 tests/Fixtures/categories.csv とは 改行コードだけが異なる。参照しているコードは無い。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough受注税計算と問い合わせ確認ページを修正し、トランザクション、管理画面、商品、カート、購入関連の未完了テストを実行可能な検証へ更新しました。複数配送、CSV、配送編集、販売種別変更などのシナリオも現行フローへ合わせています。 ChangesEC機能とテスト更新
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Customer
participant CartValidationTest
participant CartService
participant CheckoutFlow
Customer->>CartValidationTest: 異なる販売種別の商品を投入
CartValidationTest->>CartService: カート分割状態を確認
CartService-->>CartValidationTest: 分割されたカートを返却
CartValidationTest->>CheckoutFlow: 各カートの購入導線を確認
CheckoutFlow-->>Customer: 独立したチェックアウト導線
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Eccube/Tests/Web/ForgotControllerTest.php (1)
55-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winメソッドの戻り値に型宣言を追加してください。
コーディング規約（「PHP の引数と戻り値には型宣言を付け...」）に従い、テストメソッドには戻り値の型として
: voidを指定することをお勧めします。不適切な: neverを削除した際に、型宣言自体が失われています。♻️ 修正案
- public function testIndexWithPostAndVerify() + public function testIndexWithPostAndVerify(): void🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Eccube/Tests/Web/ForgotControllerTest.php` around lines 55 - 56, テストメソッド testIndexWithPostAndVerify に戻り値の型宣言 : void を追加してください。誤った : never は使用せず、既存のテスト処理は変更しないでください。Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/Eccube/Tests/Web/ForgotControllerTest.php`:
- Around line 55-56: テストメソッド testIndexWithPostAndVerify に戻り値の型宣言 : void
を追加してください。誤った : never は使用せず、既存のテスト処理は変更しないでください。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5215dbc3-772f-4d5e-bf97-fb08b7c0e9cf
⛔ Files ignored due to path filters (1)
tests/Eccube/Tests/Web/Admin/Product/categories.csvis excluded by!**/*.csv
📒 Files selected for processing (28)
src/Eccube/Controller/ContactController.phpsrc/Eccube/Entity/Order.phptests/Eccube/Tests/Command/CommandTestCase.phptests/Eccube/Tests/Command/CsvLoaderCommandTest.phptests/Eccube/Tests/Entity/OrderTest.phptests/Eccube/Tests/EventListener/TransactionListenerTest.phptests/Eccube/Tests/Form/Type/Admin/OrderItemTypeTest.phptests/Eccube/Tests/Form/Type/Install/Step1TypeTest.phptests/Eccube/Tests/Service/EntityProxyServiceTest.phptests/Eccube/Tests/Service/Payment/PaymentMethodTest.phptests/Eccube/Tests/Service/PluginServiceWithEntityExtensionTest.phptests/Eccube/Tests/Service/PurchaseFlow/PurchaseFlowTest.phptests/Eccube/Tests/Service/PurchaseFlow/ValidatableItemProcessorTest.phptests/Eccube/Tests/Transaction/TransactionListenerTest.phptests/Eccube/Tests/Web/Admin/Order/EditControllerTest.phptests/Eccube/Tests/Web/Admin/Order/EditControllerWithMultipleTest.phptests/Eccube/Tests/Web/Admin/Order/OrderControllerTest.phptests/Eccube/Tests/Web/Admin/Product/CategoryControllerTest.phptests/Eccube/Tests/Web/Admin/Product/CsvImportControllerTest.phptests/Eccube/Tests/Web/Admin/Product/ProductControllerTest.phptests/Eccube/Tests/Web/Admin/Store/TemplateControllerTest.phptests/Eccube/Tests/Web/CartValidationTest.phptests/Eccube/Tests/Web/ContactControllerTest.phptests/Eccube/Tests/Web/ForgotControllerTest.phptests/Eccube/Tests/Web/ShoppingControllerTest.phptests/Eccube/Tests/Web/ShoppingControllerWithMultipleNonmemberTest.phptests/Eccube/Tests/Web/ShoppingControllerWithMultipleTest.phptests/Eccube/Tests/Web/ShoppingControllerWithNonmemberTest.php
💤 Files with no reviewable changes (6)
- tests/Eccube/Tests/Form/Type/Install/Step1TypeTest.php
- tests/Eccube/Tests/Command/CsvLoaderCommandTest.php
- tests/Eccube/Tests/Service/PurchaseFlow/PurchaseFlowTest.php
- tests/Eccube/Tests/Command/CommandTestCase.php
- tests/Eccube/Tests/Transaction/TransactionListenerTest.php
- tests/Eccube/Tests/Service/PluginServiceWithEntityExtensionTest.php
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.4 #6945 +/- ##
==========================================
+ Coverage 75.79% 77.12% +1.33%
==========================================
Files 546 546
Lines 27136 27140 +4
==========================================
+ Hits 20569 20933 +364
+ Misses 6567 6207 -360
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要(Overview・Refs Issue)
#6926 のレビューで
EditControllerWithMultipleTestがsetUp()のmarkTestIncomplete()によりまったく実行されていないことが判明したのを受け、同種のテストを全件棚卸ししました。調査の結果、マーカーの理由の多くが誤りか陳腐化しており、「未実装だからスキップ」と書かれたものの大半は機能は 4.4 に実装済みで、テスト側が旧仕様・旧 API のまま取り残されているだけでした。さらに、死んだテストの陰に実装側の不具合が 2 件隠れていました。
markTestIncompleteを 51 件 → 13 件に削減しています（残 13 件のうち Agent Commerce の 12 件は仕様トレーサビリティ用の意図的なマーカーです）。ProductControllerTesttests/Web/Admin/Order/CartValidationTestTransactionListener方針(Policy)
マーカーを外して実際に実行し、失敗内容から根因を特定したうえで、根因ごとにコミットを分けました。
実装に関する補足(Appendix)
src/の変更は 2 ファイル 12 行のみですOrder::getTotalByTaxRate()に null ガードを追加 (src/Eccube/Entity/Order.php)$roundingTypes[$rate]->getId()を null ガードなしで呼んでおり、RoundingType未設定の課税明細が 1 件でもあるとCall to a member function getId() on nullになります（OrderItem::getRoundingType()は?RoundingType）。対になる
getTaxByTaxRate()には 2024-03-11 の f653af6 "Fix phpstan" で同じガードが入っており、getTotalByTaxRate()だけが漏れていました。単なる防御ではなく実害の修正でもあります。
OrderPdfServiceはと両者を突き合わせるため、集計対象の税率がずれると未定義キーを引きます。本修正で両者の税率キーが常に一致します。
通常は PurchaseFlow の
TaxProcessorが必ず設定するため顕在化しにくいですが、明細を直接組み立てるカスタマイズ・プラグインからは到達し得ます。お問い合わせ確認ページの
<title>を修正 (src/Eccube/Controller/ContactController.php)contactとcontact_confirmが同一パス/contactに割り当てられているため、ルータは常にcontactにマッチします。TwigInitializeListenerは_routeから引いた Page 名を twig グローバルtitleに設定するので、確認ページでも<title>が「お問い合わせ(入力ページ)」のままでした。コントローラが render に渡すPage(contact_confirm)はmeta_tagsには効いており（noindexは正しく出力される）、titleだけが不整合な状態です。default_frame.twigの<title>はsubtitleをtitleより優先するため、subtitleに Page 名を渡して上書きします。ProductControllerの'subtitle' => $Product->getName()と同じ手法です。ルーティングは変更していません。 パスを分けると公開 URL が変わり後方互換性を壊すためです。
棚卸しで判明した主な事実
CartService::lock()/unlock()は 4 系に存在しません。「カートのアンロック対応」系のマーカーは廃止された概念を前提にしていたため、該当テストを整理しました。SearchProductTypeに実装済みです。「検索項目(公開・非公開・在庫内)の実装完了後に実施」は陳腐化しており、テストが「在庫なし」をリンクで探していた（現行は select）だけでした。admin_shipping_update_order_statusは現存し、受注一覧の一括ステータス更新から実際に使われています。「使用していないルーティングのためスキップ」は事実誤認で、失敗原因は NEW 受注が無いフィクスチャ不足でした。EccubeConfig::offsetSet()は設計上LogicExceptionを投げる読み取り専用です。設定を書き換える CSV 雛形テストの手法は原理的に成立しないため、期待値側を出力エンコーディングに合わせる方式へ変えました。expectOutputString/expectOutputRegexは原理的にマッチしません。HttpKernelBrowser::filterResponse()がob_start()→sendContent()→ob_end_clean()で StreamedResponse の出力を自前バッファに吸い上げて捨てるためです。$this->client->getInternalResponse()->getContent()へ移行しました。EditControllerWithMultipleTestの 12 テストのうち 8 つはEditControllerTestの重複でした。固有価値のある複数配送のカバレッジのみ現行の作法で書き直しています。テスト側の品質問題も修正しました
OrderControllerTest::testSimpleUpdateOrderStatusWithSendMailが空振りで green でした。findBy(['OrderStatus' => NEW])が 0 件を返しforeachが回らず、アサーションが 1 つも実行されていませんでした。CsvImportControllerTestは実行するだけでリポジトリを汚していました。fixture のコピー先が__DIR__（git 管理下）だったためです。sys_get_temp_dir()に変更し、併せて 8740f32 で誤ってコミットされていた成果物tests/Eccube/Tests/Web/Admin/Product/categories.csvを削除しました（コピー元tests/Fixtures/categories.csvとは改行コードだけが異なり、参照コードはありません）。テスト（Test)
Order::getTotalByTaxRate()のガードを外す →Call to a member function getId() on nullContactControllerのsubtitleを外す →EC-CUBE SHOP / お問い合わせ(入力ページ)で落ちるItemValidator::execute()のhandle()を潰す →testValidateCartFailが落ちるEditControllerWithMultipleTestは fix: 受注編集で削除明細のスロット再利用時に税表示区分・規格情報が引き継がれる問題を修正 (#6444) #6926 のassociateOrderAndShippingを一時適用しnull === getShipping()ガードを外すと、第2配送先の明細が第1配送先へ集約される退行を検出することを確認（本 PR 単体では現行実装のままのため空振りしませんが、fix: 受注編集で削除明細のスロット再利用時に税表示区分・規格情報が引き継がれる問題を修正 (#6444) #6926 マージ後の回帰ネットとして機能します）[OK] No errors/ PHP-CS-Fixer 0 件 / Rector 冪等。相談（Discussion）
CartValidationTest8 件・ShoppingControllerWithNonmemberTest2 件・ShoppingControllerTest1 件は、購入フロー変更（scenarioConfirm後のリダイレクト消失）とマイページ履歴 UI の追従が必要で、購入フロー全体の調査が先行します。本 PR では理由コメントのみ実態に合わせて訂正しました。別 issue での対応が妥当と考えています。Step4TypeTest::testInvalidDataの 1 件は、Web インストーラが MySQL/PostgreSQL で 500 になる 4.4 の回帰を検出します（2025-10-22 の fbad0fd が誤った PHPDoc を根拠に$data→array $dataへ変換したもの）。修正とセットで別 PRを予定しています。UpdateSchemaDoctrineCommandTestは PostgreSQL 限定のため、削除したPluginServiceWithEntityExtensionTestが持っていた uninstall 系と「無効プラグインの Trait は proxy に含まれない」の検証は、SQLite/MySQL では未カバーのままです。DB 非依存なEntityProxyServiceTest側に寄せる余地があります。マイナーバージョン互換性保持のための制限事項チェックリスト
subtitleの追加のみ）レビュワー確認項目
🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
不具合修正
品質改善