Skip to content

fix: PHPUnit でテスト後半が could not find MDB2 instance#1402

Draft
seasoftjapan wants to merge 1 commit into
masterfrom
seasoft-1401
Draft

fix: PHPUnit でテスト後半が could not find MDB2 instance#1402
seasoftjapan wants to merge 1 commit into
masterfrom
seasoft-1401

Conversation

@seasoftjapan
Copy link
Copy Markdown
Contributor

@seasoftjapan seasoftjapan commented May 26, 2026

fix #1401

Summary by CodeRabbit

  • Tests
    • テスト初期化処理を改善し、データベースプール管理に関する条件付きクリーンアップロジックを追加しました。これにより、テスト実行間での不要な状態保持を防止できます。

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ee44fb04-a67e-44f1-8b68-c5a23cf34761

📥 Commits

Reviewing files that changed from the base of the PR and between fbc45f7 and 941dee3.

📒 Files selected for processing (1)
  • tests/class/Common_TestCase.php

📝 Walkthrough

Walkthrough

Common_TestCase::setUp() にMDB2プールの初期化クリアロジックを追加し、グローバルデータベースプールが空かつキャッシュされたプールエントリが存在する場合にそれを削除します。

Changes

テスト初期化でのMDB2プール状態クリア

Layer / File(s) Summary
MDB2 Pool Cleanup in Test Setup
tests/class/Common_TestCase.php
setUp() メソッドに条件付きロジックを追加し、前のテスト実行から保持されたMDB2プールインスタンスをクリアして、複数のテスト実行時における相互汚染を防止します。

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested labels

bug:Low

Suggested reviewers

  • nanasess

Poem

🐰 テストの迷いを払い、
プールを清めてさっぱりと、
MDB2 の幽霊を退治して、
次のテストへ元気に駆ける。
バグさようなら、成功おはよう!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed タイトルはPHPUnitテストで発生するMDB2インスタンス未検出エラーの修正を明確に示しており、変更の主要な目的に完全に関連しています。
Linked Issues check ✅ Passed プルリクエストは#1401で報告された後半のテスト実行時のMDB2インスタンス未検出エラーに対処するため、Common_TestCase::setUp()にプールインスタンスの初期化破棄ロジックを追加しており、要件を満たしています。
Out of Scope Changes check ✅ Passed 変更内容は#1401で報告されたテスト実行時のMDB2エラーに対処するためのテスト初期化処理の追加であり、スコープ外の変更は含まれていません。

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch seasoft-1401

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@seasoftjapan seasoftjapan enabled auto-merge May 26, 2026 10:53
@codecov
Copy link
Copy Markdown

codecov Bot commented May 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.21%. Comparing base (fbc45f7) to head (941dee3).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1402      +/-   ##
==========================================
+ Coverage   55.88%   56.21%   +0.32%     
==========================================
  Files          87       87              
  Lines       11090    11090              
==========================================
+ Hits         6198     6234      +36     
+ Misses       4892     4856      -36     
Flag Coverage Δ
tests 56.21% <ø> (+0.32%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nanasess
Copy link
Copy Markdown
Contributor

根本原因の調査結果

ghcr.io/ec-cube/ec-cube2-php:8.3-apache + PostgreSQL 環境でデバッグしたところ、$GLOBALS['_MDB2_databases'] が消失する真因を特定できました。共有します。

真因

PHPUnit 9 の backupGlobals="true" (phpunit.xml.dist の設定) + Common_TestCase を継承しないテストの組み合わせ が原因です。

具体的な流れ:

  1. phpunit.xml.dist:7backupGlobals="true" がグローバル設定されている。
  2. Common_TestCase$backupGlobalsExcludeList = ['_MDB2_databases', '_MDB2_dsninfo_default'] で MDB2 関連を保護しているが、以下のテストは \PHPUnit\Framework\TestCase を直接継承しており、excludeList が空:
    • tests/class/module/HTTP/HTTP_Request_CompatibilityTest.php
    • tests/class/module/HTTP/HTTP_Request_GuzzleTest.php
    • tests/class/module/Net/Net_URLTest.php
    • tests/class/module/Net/Net_URL_CompatibilityTest.php
  3. これらのテスト実行時:
    • Snapshot 時 (sebastian/global-state/src/Snapshot.php:267-): $GLOBALS['_MDB2_databases'] を serialize しようとするが、内部の MDB2 接続オブジェクトがリソース (DB ハンドル) を保持しているため canBeSerialized()false を返し、スナップショットに含まれない
    • Restore 時 (sebastian/global-state/src/Restorer.php:51-72): excludeList に _MDB2_databases が無く、スナップショットにも無い → array_key_exists 分岐の elseunset($GLOBALS['_MDB2_databases']) が実行される。
  4. 以降のテストで $GLOBALS['_MDB2_databases'] が完全に消失。SC_Query_Ex::$arrPoolInstance に残っている古いインスタンスを使うと、内部の MDB2::singleton() 系で参照失敗 → 「could not find MDB2 instance」。

Modifier_ScriptEscapeTest@seasoftjapan さんが過去コミット 7631399@backupGlobals disabled を付けて回避していたのと 同じパターン です。Seasoft さんが「以前にも類似事象に遭遇している」と書かれていたのはこれだと思われます。

検証ログ (修正前、フルテスト)

Common_TestCase::setUp() / tearDown() に計測コードを仕込んで $GLOBALS['_MDB2_databases'] の状態を追ったところ、消失の瞬間を特定できました:

[OK_1 pool=2] SC_Helper_TaxRule_setTaxRuleTest::testSetDelflgOfSetTaxRule  ← 直前まで正常
[UNSET pool=2] HTTP_RequestTest::testConstructorSetsDefaults                ← ここで突如 UNSET
[UNSET pool=2] XML_Serializer_Objects_TestCase::testEmptyObject
[UNSET pool=2] ...
[UNSET pool=2] SC_Utils_sfGetAddressTest::test住所がヒットしない場合空の配列が返る  ← #1401 のエラー発生

HTTP_RequestTest 単独や SC_Helper_TaxRule + Calendar_Test + HTTP_RequestTest--filter で組み合わせて実行しても UNSET にならず、HTTP_Request_CompatibilityTest を間に挟むと UNSET になる ことで犯人を特定しました。

CI で顕在化していない理由

.github/workflows/unit-tests.yml:78-83 で意図的に分割実行されているためです:

- name: Run to PHPUnit
  run: docker compose exec -T ec-cube php data/vendor/bin/phpunit --exclude-group classloader,mysql_prepare
- name: Run to PHPUnit mysql_prepare
  # XXX 連続してテストを実行すると、何故か MySQL の prepare statement に失敗するため個別に実行する
  run: docker compose exec -T ec-cube php data/vendor/bin/phpunit --group mysql_prepare

#1401 でエラーになる SC_Utils_sfGetAddressTestSC_Utils_sfGetProductClassIdTest は両方とも @group mysql_prepare が付いており:

  • メイン実行では --exclude-group ...mysql_prepare除外 されてエラーが見えない
  • --group mysql_prepare 単独実行では HTTP_Request_CompatibilityTest 等が走らないので _MDB2_databases が unset されない

ワークフローの「XXX 連続してテストを実行すると…」というコメントは、まさにこの症状の対症療法だったと思われます。

推奨される根本対処の diff

PR #1402 の対症療法はそのままマージで問題ありませんが、追加で以下の annotation を入れると、根本原因側を解消できます (Modifier_ScriptEscapeTest への対処と同じ手法):

diff --git a/tests/class/module/HTTP/HTTP_Request_CompatibilityTest.php b/tests/class/module/HTTP/HTTP_Request_CompatibilityTest.php
--- a/tests/class/module/HTTP/HTTP_Request_CompatibilityTest.php
+++ b/tests/class/module/HTTP/HTTP_Request_CompatibilityTest.php
@@ -9,6 +9,8 @@ require_once __DIR__.'/Request_Legacy.php';
  *
  * Compares the new Guzzle-based implementation with the legacy implementation
  * to ensure backward compatibility of the API.
+ *
+ * @backupGlobals disabled
  */
 class HTTP_Request_CompatibilityTest extends \PHPUnit\Framework\TestCase

diff --git a/tests/class/module/HTTP/HTTP_Request_GuzzleTest.php b/tests/class/module/HTTP/HTTP_Request_GuzzleTest.php
--- a/tests/class/module/HTTP/HTTP_Request_GuzzleTest.php
+++ b/tests/class/module/HTTP/HTTP_Request_GuzzleTest.php
@@ -7,6 +7,8 @@ require_once $HOME.'/data/module/HTTP/Request.php';
  * HTTP_Request compatibility tests
  *
  * Tests to verify backward compatibility of the Guzzle-based HTTP_Request implementation.
+ *
+ * @backupGlobals disabled
  */
 class HTTP_Request_GuzzleTest extends \PHPUnit\Framework\TestCase

diff --git a/tests/class/module/Net/Net_URLTest.php b/tests/class/module/Net/Net_URLTest.php
--- a/tests/class/module/Net/Net_URLTest.php
+++ b/tests/class/module/Net/Net_URLTest.php
@@ -7,6 +7,8 @@ require_once $HOME.'/data/module/Net/URL.php';
  * Net_URL compatibility tests
  *
  * Tests to verify backward compatibility of the Guzzle-based Net_URL implementation.
+ *
+ * @backupGlobals disabled
  */
 class Net_URLTest extends \PHPUnit\Framework\TestCase

diff --git a/tests/class/module/Net/Net_URL_CompatibilityTest.php b/tests/class/module/Net/Net_URL_CompatibilityTest.php
--- a/tests/class/module/Net/Net_URL_CompatibilityTest.php
+++ b/tests/class/module/Net/Net_URL_CompatibilityTest.php
@@ -9,6 +9,8 @@ require_once __DIR__.'/URL_Legacy.php';
  *
  * Compares the new Guzzle-based implementation with the legacy implementation
  * to ensure backward compatibility.
+ *
+ * @backupGlobals disabled
  */
 class Net_URL_CompatibilityTest extends \PHPUnit\Framework\TestCase

検証結果

ghcr.io/ec-cube/ec-cube2-php:8.3-apache + PostgreSQL で確認:

状態 結果
修正なし (master) Tests: 1798, Errors: 11 (#1401 再現)
PR #1402 のみ Tests: 1798, Errors: 0
本コメントの annotation 4 件のみ (PR #1402 なし) Tests: 1798, Errors: 0

提案

  • PR fix: PHPUnit でテスト後半が could not find MDB2 instance #1402 はマージ OK — 確実に効く対症療法として有用。
  • フォローアップで上記 annotation 追加を検討するのが望ましいです。これがあれば unit-tests.yml--exclude-group mysql_prepare 分割の理由 (「XXX 連続してテストを実行すると…」) も解消され、ワークフロー簡素化の余地が生まれます。
  • 長期的には \PHPUnit\Framework\TestCase を直接継承する代わりに Common_TestCase (または backupGlobalsExcludeList 設定済みの基底クラス) を継承するルール化、もしくは phpunit.xml.distbackupGlobals 設定見直しも検討の価値があります。

🤖 Generated with Claude Code

@seasoftjapan seasoftjapan marked this pull request as draft May 27, 2026 03:35
auto-merge was automatically disabled May 27, 2026 03:35

Pull request was converted to draft

@seasoftjapan
Copy link
Copy Markdown
Contributor Author

  • 長期的には \PHPUnit\Framework\TestCase を直接継承する代わりに Common_TestCase (または backupGlobalsExcludeList 設定済みの基底クラス) を継承するルール化、もしくは phpunit.xml.distbackupGlobals 設定見直しも検討の価値があります。

Common_TestCase を使う形で、#1403 を作成しました。
それで問題無いならば、本 PR の取り込みは見送って良さそうに思います。

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.

PHPUnit でテスト後半が could not find MDB2 instance

2 participants