feat: add environment variable support for test snapshot modes #5
Conversation
WalkthroughCIワークフローに Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI workflow
participant TR as Test Runner
participant PTC as PagesTestCase
participant FS as Fixtures/Cache
note over CI: CI sets EMBED_STRICT_CACHE=1
CI->>TR: start tests
TR->>PTC: run assertEmbed(test)
PTC->>PTC: getCacheMode(), getFixturesMode(), isStrictMode()
alt strict mode (EMBED_STRICT_CACHE=1)
PTC->>FS: check fixture exists
FS-->>PTC: missing
PTC-->>TR: fail test (strict)
else update fixtures mode
PTC->>FS: write fixture
FS-->>PTC: saved
PTC-->>TR: skip test (fixture updated)
else default/non-strict
PTC->>FS: check fixture
alt fixture exists
FS-->>PTC: returns fixture
PTC-->>TR: assert using fixture
else missing
PTC->>FS: write fixture
FS-->>PTC: saved
PTC-->>TR: skip test (new fixture)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20分
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/PagesTestCase.php (1)
125-142:UPDATE_EMBED_SNAPSHOTSとEMBED_STRICT_CACHEの併用時に、README の仕様通りに動作していません検証結果:
readData()は存在しないフィクスチャに対してnullを返すことを確認しました（line 163）- README の「
UPDATE_EMBED_SNAPSHOTSが優先」という仕様を確認しました（line 376）- 現在のコード: strict チェック（line 126-127）が UPDATE チェック（line 131）より先に実行されるため、両フラグ設定時にフィクスチャが無いとテストが失敗し、UPDATE モードに進みません
これは仕様違反です。提案された修正（UPDATE モード判定を先に実行する順序変更と
=== nullの明示化）を適用してください：- // In strict mode, fail if fixture is missing - if (!$expected && self::isStrictMode()) { - $this->fail("Fixture not found for {$url}"); - } - - // Update mode: save fixture and skip - if (self::getFixturesMode() === 1) { + // Update mode: always save fixture and skip assertions + if (self::getFixturesMode() === 1) { self::writeData($uri, $data); echo PHP_EOL."Save fixture: {$url}"; $this->markTestSkipped('Skipped assertion for '.$url); - } elseif (!$expected) { - // Missing fixture in non-strict mode: create and skip + + return; + } + + // In strict mode, fail if fixture is missing + if ($expected === null && self::isStrictMode()) { + $this->fail("Fixture not found for {$url}"); + } + + if ($expected === null) { + // Missing fixture in non-strict mode: create and skip self::writeData($uri, $data); echo PHP_EOL."Save fixture: {$url}"; $this->markTestSkipped('Skipped assertion for '.$url); + + return; + } + + $this->assertEquals($expected, $data, $url); - } else { - $this->assertEquals($expected, $data, $url); - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
.github/workflows/test.yml(1 hunks)README.md(1 hunks)tests/PagesTestCase.php(3 hunks)
🔇 Additional comments (5)
.github/workflows/test.yml (1)
43-46: CI のテストステップへのEMBED_STRICT_CACHE=1設定は実装／ドキュメントと整合しています
composer test実行時にのみEMBED_STRICT_CACHE=1を付与しており、PagesTestCase::getCacheMode()と README の「For CI environments」の説明どおり、CI ではキャッシュ欠如を検知して落とす構成になっていて問題なさそうです。tests/PagesTestCase.php (4)
18-40: CACHE / FIXTURES 定数のモード定義と env 上書きの説明が明確で良いですキャッシュモード（-1〜2）とフィクスチャモード（0/1）の意味、および
UPDATE_EMBED_SNAPSHOTS・EMBED_STRICT_CACHEによる上書きルールがコメントで整理されており、テスト戦略の意図が読み取りやすくなっています。実装とも整合しているので、このままで問題ないと思います。
65-82: getCacheMode() による env 起点のキャッシュモード切り替えは意図どおりです
UPDATE_EMBED_SNAPSHOTS優先で2（キャッシュ上書き）、それ以外でEMBED_STRICT_CACHEによる-1（キャッシュ必須）、いずれも未設定時はstatic::CACHEという分岐になっており、README の説明および CI 設定（EMBED_STRICT_CACHE=1）と噛み合っています。特に懸念点はありません。
83-95: getFixturesMode() の実装もシンプルで UPDATE_EMBED_SNAPSHOTS の用途に合っています
UPDATE_EMBED_SNAPSHOTSがセットされていればフィクスチャ更新モード（1）、それ以外はstatic::FIXTURESにフォールバックするだけの構造で、Snapshot 更新フローの意図を過不足なく表現できていると思います。
96-102: strict モードフラグと getEmbed() でのキャッシュモード適用は妥当です
isStrictMode()がEMBED_STRICT_CACHEの有無のみを見て bool を返し、getEmbed()側ではFileClientに対してself::getCacheMode()を渡す構造になっており、キャッシュ戦略の切り替え点が一箇所に集約されていて分かりやすいです。FileClientとの責務分担も明確で、ここについてはそのままで良さそうです。Also applies to: 110-112
| ## Testing | ||
|
|
||
| ### Running tests | ||
|
|
||
| ```bash | ||
| composer test | ||
| # or | ||
| ./vendor/bin/phpunit | ||
| ``` | ||
|
|
||
| ### Snapshot modes | ||
|
|
||
| The test suite uses cached HTTP responses and fixtures to avoid network requests during testing. You can control this behavior using environment variables: | ||
|
|
||
| | Environment Variable | Description | | ||
| |---------------------|-------------| | ||
| | `UPDATE_EMBED_SNAPSHOTS=1` | Fetch from network and update both cache and fixtures | | ||
| | `EMBED_STRICT_CACHE=1` | Fail if cache or fixture doesn't exist (useful for CI) | | ||
|
|
||
| By default (no environment variables set), tests read from cache and generate missing files automatically. | ||
|
|
||
| **Note:** If both `UPDATE_EMBED_SNAPSHOTS` and `EMBED_STRICT_CACHE` are set, `UPDATE_EMBED_SNAPSHOTS` takes precedence. | ||
|
|
||
| ### Cache structure | ||
|
|
||
| The test framework uses two types of cached data: | ||
|
|
||
| - **Response cache** (`tests/cache/`): Cached HTTP responses from external sites | ||
| - **Fixtures** (`tests/fixtures/`): Expected test results (metadata extracted from cached responses) | ||
|
|
||
| ### How to update cache | ||
|
|
||
| #### When a website changes its HTML structure | ||
|
|
||
| If a website updates its HTML and you need to update the cached response and fixture: | ||
|
|
||
| ```bash | ||
| # Update cache and fixture for a specific test | ||
| UPDATE_EMBED_SNAPSHOTS=1 ./vendor/bin/phpunit --filter testYoutube | ||
| ``` | ||
|
|
||
| #### When adding a new test | ||
|
|
||
| After adding a new URL to test: | ||
|
|
||
| ```bash | ||
| # This will fetch the response and create both cache and fixture | ||
| UPDATE_EMBED_SNAPSHOTS=1 ./vendor/bin/phpunit --filter testNewSite | ||
| ``` | ||
|
|
||
| #### Update all caches at once | ||
|
|
||
| To refresh all cached responses and fixtures from the network: | ||
|
|
||
| ```bash | ||
| UPDATE_EMBED_SNAPSHOTS=1 ./vendor/bin/phpunit | ||
| ``` | ||
|
|
||
| #### For CI environments | ||
|
|
||
| Ensure all tests run strictly from cache (fail if any cache is missing): | ||
|
|
||
| ```bash | ||
| EMBED_STRICT_CACHE=1 ./vendor/bin/phpunit | ||
| ``` | ||
|
|
||
| --- |
There was a problem hiding this comment.
🧩 Analysis chain
Testing セクションは分かりやすいですが、env 併用時の挙動説明が実装と一部ずれています
Snapshot modes の Note で「UPDATE_EMBED_SNAPSHOTS と EMBED_STRICT_CACHE の両方がセットされた場合は UPDATE_EMBED_SNAPSHOTS が優先」とありますが、現状の tests/PagesTestCase::assertEmbed() では両方がセットされ、かつフィクスチャが存在しない場合に「strict チェックの fail」が先に走り、フィクスチャ更新には到達しません。
README どおりの挙動にするなら、コード側で UPDATE_EMBED_SNAPSHOTS を完全に優先するよう（たとえば getFixturesMode() の判定を先に実行し、strict チェックより前にフィクスチャを書き出して skip するなど）調整するか、逆に現在の実装を正とするなら Note の文言を修正しておくと読者が混乱しにくいと思います。この点は tests/PagesTestCase.php 側への修正案コメントも併せてご確認ください。
🏁 Script executed:
#!/bin/bash
# Locate and examine PagesTestCase.php, focusing on assertEmbed() method
fd -e php -p "*PagesTestCase*" | head -5Length of output: 294
🏁 Script executed:
#!/bin/bash
# Search for the assertEmbed method and surrounding context with both env var checks
rg -n -A 30 "function assertEmbed" --type phpLength of output: 1985
🏁 Script executed:
#!/bin/bash
# Also check for EMBED_STRICT_CACHE and UPDATE_EMBED_SNAPSHOTS references in tests
rg -n "EMBED_STRICT_CACHE|UPDATE_EMBED_SNAPSHOTS" --type php -B 2 -A 2Length of output: 2000
README.md の Testing セクション記述と tests/PagesTestCase.php の実装が不一致である点が確認されました
検証結果、以下が明らかになりました：
tests/PagesTestCase.php の assertEmbed() メソッドでは：
- 126～128行目：
if (!$expected && self::isStrictMode())が先に実行され、フィクスチャが存在しない場合にここで失敗 - 131～134行目：
if (self::getFixturesMode() === 1)でフィクスチャ更新が実行されるはずだが、上記の厳密モードチェックに引っかかると到達しない
つまり、UPDATE_EMBED_SNAPSHOTS=1 と EMBED_STRICT_CACHE=1 の両方がセットされ、フィクスチャが欠落している場合、README で謳う「UPDATE_EMBED_SNAPSHOTS が優先」という動作は実現されていません。
対応方法：
- コード側修正：
getFixturesMode()の判定を先に実行し、UPDATE_EMBED_SNAPSHOTS が有効な場合はフィクスチャを作成してから厳密モードチェックをスキップ - README 修正：現在の実装に合わせ、Note 文言を「厳密モードが優先される」に修正
いずれかの対応を行い、README とコード実装を一致させてください。
🤖 Prompt for AI Agents
README.md around lines 355-421: the README claims UPDATE_EMBED_SNAPSHOTS takes
precedence over EMBED_STRICT_CACHE but tests/PagesTestCase.php currently checks
strict-mode before fixtures-mode (see lines ~126-134), causing the opposite
behavior; update tests/PagesTestCase.php so getFixturesMode() is evaluated
before the strict-mode check—if fixtures mode indicates update
(UPDATE_EMBED_SNAPSHOTS=1) create/update the fixture first and only then skip or
re-evaluate the strict-mode failure path, ensuring that when both env vars are
set the snapshot-update path runs; alternatively, if you prefer keeping current
code, change the README note to state that strict mode takes precedence.
Add UPDATE_EMBED_SNAPSHOTS environment variable to update both HTTP response cache and fixtures together when running tests. Changes: - Add UPDATE_EMBED_SNAPSHOTS=1 to fetch from network and update snapshots - Add EMBED_STRICT_CACHE=1 to fail if cache is missing (for CI) - Update GitHub Actions workflow to use EMBED_STRICT_CACHE - Add comprehensive documentation for snapshot testing in README Usage: UPDATE_EMBED_SNAPSHOTS=1 ./vendor/bin/phpunit --filter testYoutube
uzulla
force-pushed
the
feature/snapshot-environment-variables
branch
from
c88c244 to
ef4df71
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/PagesTestCase.php (1)
125-142: 条件チェックの順序に問題がありますStrictモードのチェック（125-128行目）が更新モードのチェック（131-135行目）より前にあるため、
UPDATE_EMBED_SNAPSHOTS=1とEMBED_STRICT_CACHE=1の両方が設定されている場合、フィクスチャが存在しないとテストが失敗してしまいます。更新モードでは新しいフィクスチャを作成することが目的なので、この動作は意図に反します。更新モードを最初にチェックするよう順序を変更してください：
$uri = $extractor->getRequest()->getUri(); $data = self::getData($extractor); $expected = self::readData($uri); - // In strict mode, fail if fixture is missing - if (!$expected && self::isStrictMode()) { - $this->fail("Fixture not found for {$url}"); - } - // Update mode: save fixture and skip if (self::getFixturesMode() === 1) { self::writeData($uri, $data); echo PHP_EOL."Save fixture: {$url}"; $this->markTestSkipped('Skipped assertion for '.$url); + } elseif (!$expected && self::isStrictMode()) { + // In strict mode, fail if fixture is missing + $this->fail("Fixture not found for {$url}"); } elseif (!$expected) { // Missing fixture in non-strict mode: create and skip self::writeData($uri, $data);これにより、更新モードが優先され、strictモードの矛盾した設定でも正しく動作します。
♻️ Duplicate comments (1)
README.md (1)
376-376: 環境変数の優先順位の説明が実装と矛盾している（既知の問題）。このノートの説明「
UPDATE_EMBED_SNAPSHOTSがEMBED_STRICT_CACHEより優先される」と、tests/PagesTestCase.phpの実装が一致していません。過去のレビューで指摘されたとおり、実装では strict モードチェック（isStrictMode()の判定）がフィクスチャ更新処理より先に実行されるため、両方の環境変数が設定された場合、実際には strict モードが優先されます。以下のいずれかの対応を行ってください：
- 実装を修正する場合：
tests/PagesTestCase.phpのassertEmbed()メソッドでgetFixturesMode()の判定を先に実行し、UPDATE_EMBED_SNAPSHOTS が有効な場合はフィクスチャを更新してから strict モードチェックをスキップする- README を修正する場合：このノートを「strict モードが優先される」に変更する
過去のレビュー指摘（lines 355-421）を参照してください。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
.github/workflows/test.yml(1 hunks)README.md(1 hunks)tests/PagesTestCase.php(3 hunks)
🔇 Additional comments (4)
README.md (1)
357-375: Testing セクションの構成と説明品質は良好です。Running tests、Cache structure、How to update cache のセクションは論理的に構成されており、実用的で明確な例が提供されています。特に環境変数の表形式での説明と、具体的なテスト更新シナリオ（特定テスト、新規テスト、全体更新、CI環境）をカバーしている点は好評です。
上述の優先順位に関する矛盾が解決されれば、このドキュメントは有用なリソースになります。
Also applies to: 378-419
.github/workflows/test.yml (1)
45-46: LGTM!CI環境でキャッシュとフィクスチャの存在を確認する適切な設定です。これによりテストの再現性が向上します。
tests/PagesTestCase.php (2)
18-39: ドキュメントが明確で分かりやすい各モードの動作と環境変数による上書きが適切に説明されています。
111-111: 動的な動作を有効にする適切な変更です定数から環境変数対応のメソッドへの変更により、実行時の柔軟な制御が可能になります。
| protected static function getCacheMode(): int | ||
| { | ||
| // UPDATE_EMBED_SNAPSHOTS=1 : Fetch from network and override cache | ||
| if (getenv('UPDATE_EMBED_SNAPSHOTS')) { | ||
| return 2; | ||
| } | ||
|
|
||
| // EMBED_STRICT_CACHE=1 : Fail if cache doesn't exist | ||
| if (getenv('EMBED_STRICT_CACHE')) { | ||
| return -1; | ||
| } | ||
|
|
||
| return static::CACHE; | ||
| } | ||
|
|
||
| /** | ||
| * Get fixtures mode from environment variable or class constant | ||
| */ | ||
| protected static function getFixturesMode(): int | ||
| { | ||
| // UPDATE_EMBED_SNAPSHOTS=1 : Override fixtures with new results | ||
| if (getenv('UPDATE_EMBED_SNAPSHOTS')) { | ||
| return 1; | ||
| } | ||
|
|
||
| return static::FIXTURES; | ||
| } |
There was a problem hiding this comment.
環境変数のチェックが曖昧です
getenv()はfalseまたは文字列を返すため、if (getenv('UPDATE_EMBED_SNAPSHOTS'))は空でない文字列すべてに対してtrueとなります。つまりUPDATE_EMBED_SNAPSHOTS=0でも更新モードが有効になってしまいます。
以下のように明示的に値をチェックすることをお勧めします：
- if (getenv('UPDATE_EMBED_SNAPSHOTS')) {
+ if (getenv('UPDATE_EMBED_SNAPSHOTS') === '1') {
return 2;
}同様に他の環境変数チェックにも適用してください。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected static function getCacheMode(): int | |
| { | |
| // UPDATE_EMBED_SNAPSHOTS=1 : Fetch from network and override cache | |
| if (getenv('UPDATE_EMBED_SNAPSHOTS')) { | |
| return 2; | |
| } | |
| // EMBED_STRICT_CACHE=1 : Fail if cache doesn't exist | |
| if (getenv('EMBED_STRICT_CACHE')) { | |
| return -1; | |
| } | |
| return static::CACHE; | |
| } | |
| /** | |
| * Get fixtures mode from environment variable or class constant | |
| */ | |
| protected static function getFixturesMode(): int | |
| { | |
| // UPDATE_EMBED_SNAPSHOTS=1 : Override fixtures with new results | |
| if (getenv('UPDATE_EMBED_SNAPSHOTS')) { | |
| return 1; | |
| } | |
| return static::FIXTURES; | |
| } | |
| protected static function getCacheMode(): int | |
| { | |
| // UPDATE_EMBED_SNAPSHOTS=1 : Fetch from network and override cache | |
| if (getenv('UPDATE_EMBED_SNAPSHOTS') === '1') { | |
| return 2; | |
| } | |
| // EMBED_STRICT_CACHE=1 : Fail if cache doesn't exist | |
| if (getenv('EMBED_STRICT_CACHE') === '1') { | |
| return -1; | |
| } | |
| return static::CACHE; | |
| } | |
| /** | |
| * Get fixtures mode from environment variable or class constant | |
| */ | |
| protected static function getFixturesMode(): int | |
| { | |
| // UPDATE_EMBED_SNAPSHOTS=1 : Override fixtures with new results | |
| if (getenv('UPDATE_EMBED_SNAPSHOTS') === '1') { | |
| return 1; | |
| } | |
| return static::FIXTURES; | |
| } |
🤖 Prompt for AI Agents
In tests/PagesTestCase.php around lines 68–94, the getenv() checks are too loose
(getenv returns strings) so values like "0" still evaluate true; change these
checks to explicitly compare the environment variable to the expected truthy
string(s) (e.g. strict compare to "1" or normalized lowercase in a set like
["1","true","yes"]) for UPDATE_EMBED_SNAPSHOTS and EMBED_STRICT_CACHE, and apply
the same explicit comparison for the fixtures check, ensuring the logic returns
the correct mode only when the env var is intentionally enabled.
| protected static function isStrictMode(): bool | ||
| { | ||
| return (bool) getenv('EMBED_STRICT_CACHE'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
isStrictMode()の冗長性を検討してください
isStrictMode()はEMBED_STRICT_CACHEをチェックしていますが、getCacheMode()も同じ環境変数をチェックして-1を返します。代わりにself::getCacheMode() === -1を使用することで一貫性が保たれます。
リファクタリング案：
- protected static function isStrictMode(): bool
- {
- return (bool) getenv('EMBED_STRICT_CACHE');
- }
+ protected static function isStrictMode(): bool
+ {
+ return self::getCacheMode() === -1;
+ }ただし、可読性のために現在の実装を維持する選択肢もあります。
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/PagesTestCase.php around lines 99 to 102, isStrictMode() redundantly
reads getenv('EMBED_STRICT_CACHE') while getCacheMode() already interprets the
same environment variable and returns -1 for strict mode; replace the current
implementation to return self::getCacheMode() === -1 to centralize the logic and
ensure consistency (or keep the existing implementation if you prefer explicit
readability), making sure any callers rely on the canonical getCacheMode()
behavior.
Add UPDATE_EMBED_SNAPSHOTS environment variable to update both HTTP response cache and fixtures together when running tests.
Changes:
Usage:
UPDATE_EMBED_SNAPSHOTS=1 ./vendor/bin/phpunit --filter testYoutube
Summary by CodeRabbit
リリースノート
Documentation（ドキュメント）
Tests（テスト）
Chores
✏️ Tip: You can customize this high-level summary in your review settings.