Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ jobs:

- name: Tests
run: composer test
env:
EMBED_STRICT_CACHE: 1

phpstan:
name: PHPStan Static Analysis
Expand Down
68 changes: 68 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,74 @@ Note: The built-in detectors does not require settings. This feature is only for

---

## 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
```

---
Comment on lines +355 to +421
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Testing セクションは分かりやすいですが、env 併用時の挙動説明が実装と一部ずれています

Snapshot modesNote で「UPDATE_EMBED_SNAPSHOTSEMBED_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 -5

Length 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 php

Length 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 2

Length of output: 2000


README.md の Testing セクション記述と tests/PagesTestCase.php の実装が不一致である点が確認されました

検証結果、以下が明らかになりました:

tests/PagesTestCase.phpassertEmbed() メソッドでは:

  • 126~128行目if (!$expected && self::isStrictMode())先に実行され、フィクスチャが存在しない場合にここで失敗
  • 131~134行目if (self::getFixturesMode() === 1) でフィクスチャ更新が実行されるはずだが、上記の厳密モードチェックに引っかかると到達しない

つまり、UPDATE_EMBED_SNAPSHOTS=1EMBED_STRICT_CACHE=1 の両方がセットされ、フィクスチャが欠落している場合、README で謳う「UPDATE_EMBED_SNAPSHOTS が優先」という動作は実現されていません。

対応方法

  1. コード側修正getFixturesMode() の判定を先に実行し、UPDATE_EMBED_SNAPSHOTS が有効な場合はフィクスチャを作成してから厳密モードチェックをスキップ
  2. 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.


[ico-version]: https://poser.pugx.org/embed/embed/v/stable
[ico-license]: https://poser.pugx.org/embed/embed/license
[ico-downloads]: https://poser.pugx.org/embed/embed/downloads
Expand Down
74 changes: 72 additions & 2 deletions tests/PagesTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,27 @@

abstract class PagesTestCase extends TestCase
{
/**
* Cache mode
* -1 = Read from cache (throws an exception if the cache doesn't exist)
* 0 = Read from cache (generate the files the first time)
* 1 = Read from net without overriding the cache files
* 2 = Read from net and override the cache files
*
* Can be overridden by environment variables:
* - 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 (mode -1)
*/
const CACHE = 0;

/**
* Fixtures mode
* 0 = Do not override the fixtures
* 1 = Override the fixtures
*
* Can be overridden by environment variable:
* - UPDATE_EMBED_SNAPSHOTS=1 : Update both cache and fixtures
*/
const FIXTURES = 0;

private const DETECTORS = [
Expand All @@ -42,14 +62,53 @@ abstract class PagesTestCase extends TestCase

private static Embed $embed;

/**
* Get cache mode from environment variables or class constant
*/
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;
}
Comment on lines +68 to +94
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

環境変数のチェックが曖昧です

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.

Suggested change
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.


/**
* Check if strict cache mode is enabled
*/
protected static function isStrictMode(): bool
{
return (bool) getenv('EMBED_STRICT_CACHE');
}
Comment on lines +99 to +102
Copy link

Choose a reason for hiding this comment

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

🧹 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.


private static function getEmbed(): Embed
{
if (isset(self::$embed)) {
return self::$embed;
}

$dispatcher = new FileClient(__DIR__.'/cache');
$dispatcher->setMode(static::CACHE);
$dispatcher->setMode(self::getCacheMode());

return self::$embed = new Embed(new Crawler($dispatcher), self::getExtractorFactory());
}
Expand All @@ -63,7 +122,18 @@ protected function assertEmbed(string $url)
$data = self::getData($extractor);
$expected = self::readData($uri);

if (!$expected || static::FIXTURES === 1) {
// 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) {
// 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);
Expand Down