feat: tests #313
feat: tests #313
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough設定オブジェクトを InfoFeatureFlags に解決する新モジュールを追加し、info 表示ロジックをそのフラグ参照に切り替え。起動時の設定チェックはロガー/exit 抽象化へ移行し、Jest からビルド後に Node でテストを実行するスクリプトへ変更。関連する単体テストと tsconfig の Node 型追加を含む。 Changes
Sequence Diagram(s)mermaid Startup->>ConfigChecker: performStartupConfigCheck(options) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 分
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (2)test/config-updater.test.mjs (1)
src/config-updater.ts (1)
🔇 Additional comments (15)
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 |
Summary of ChangesHello @lqvp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the feature flag management within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the feature flag logic into a new feature-flags.ts module and adds unit tests for it, which is a great improvement for maintainability and correctness. My review includes a few suggestions to further enhance the changes. I've recommended improving the new module's readability by extracting a type, fixing a minor performance issue where the new function is called redundantly, and expanding the new test suite to cover more cases. Overall, this is a solid contribution.
| assert.equal(flags.emojiCheck, false); | ||
| assert.equal(flags.maze, false); | ||
| assert.equal(flags.poll, false); | ||
| }); |
There was a problem hiding this comment.
These tests are a great start! To make the test suite more robust and cover all logic paths, I'd suggest adding a few more test cases:
- Opt-in features when explicitly enabled (e.g.,
keywordEnabled: true). - Opt-in features when explicitly disabled (e.g.,
keywordEnabled: false). - Default-enabled features when explicitly enabled (e.g.,
chartEnabled: true).
Here's an example for one of the missing cases:
test('info: opt-in features are enabled when set to true', () => {
const flags = resolveInfoFeatureFlags({
keywordEnabled: true,
reversiEnabled: true,
serverMonitoring: true,
checkEmojisEnabled: true,
mazeEnable: true,
pollEnable: true,
});
assert.equal(flags.keywordSearch, true);
assert.equal(flags.reversi, true);
assert.equal(flags.serverMonitoring, true);
assert.equal(flags.emojiCheck, true);
assert.equal(flags.maze, true);
assert.equal(flags.poll, true);
});![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/modules/info/index.ts (1)
291-318: 機能フラグの解決を最適化できます
resolveInfoFeatureFlags(config)がformatBasicFeatures(292行目) とformatGameFeatures(321行目) の両方で呼び出されています。formatSafeConfigInfoで一度だけ解決し、両方の関数に渡すことで重複を削減できます。🔎 リファクタリング案
formatSafeConfigInfo内で一度だけ機能フラグを解決し、各フォーマット関数に渡すように変更:-function formatBasicFeatures(): string { - const flags = resolveInfoFeatureFlags(config); +function formatBasicFeatures(flags: InfoFeatureFlags): string { const lines: string[] = [-function formatGameFeatures(): string { - const flags = resolveInfoFeatureFlags(config); +function formatGameFeatures(flags: InfoFeatureFlags): string { const lines: string[] = [function formatSafeConfigInfo(): string { + const flags = resolveInfoFeatureFlags(config); let configInfo = `\n⚙️ **設定情報**\n`; - configInfo += formatBasicFeatures(); - configInfo += formatGameFeatures(); + configInfo += formatBasicFeatures(flags); + configInfo += formatGameFeatures(flags); configInfo += formatPostSettings();型のインポートも追加:
-import { resolveInfoFeatureFlags } from './feature-flags.js'; +import { resolveInfoFeatureFlags, type InfoFeatureFlags } from './feature-flags.js';
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)src/modules/info/feature-flags.ts(1 hunks)src/modules/info/index.ts(2 hunks)test/info-feature-flags.test.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/modules/info/index.ts (3)
test/info-feature-flags.test.mjs (3)
flags(7-12)flags(20-24)flags(32-39)src/modules/info/feature-flags.ts (1)
resolveInfoFeatureFlags(17-39)src/config.ts (1)
config(110-110)
test/info-feature-flags.test.mjs (1)
src/modules/info/feature-flags.ts (1)
resolveInfoFeatureFlags(17-39)
🔇 Additional comments (6)
package.json (1)
12-12: テストランナーの変更を確認しましたJestから Node.js 組み込みのテストランナーへの移行は適切です。ビルドステップの追加により、テスト実行前に TypeScript のコンパイルが保証されます。
src/modules/info/index.ts (1)
10-10: 機能フラグの統合が適切に実装されています
resolveInfoFeatureFlagsのインポートと使用により、設定値の解釈が一元化され、issue #306 で報告された情報の不整合が修正されます。デフォルト有効な機能（autoWord、chart、timeSignal）と明示的なオプトイン機能の区別が明確になりました。Also applies to: 292-332
test/info-feature-flags.test.mjs (1)
1-47: テストカバレッジが適切です3つのテストケースで機能フラグの解決ロジックを包括的に検証しています:
- デフォルト有効な機能（chart、autoWord、timeSignal）が未設定時に有効になること
- デフォルト有効な機能が明示的に無効化できること
- オプトイン機能（keywordSearch、reversi等）が未設定時に無効になること
テストは実装と完全に整合しており、issue #306 の修正を検証します。
src/modules/info/feature-flags.ts (3)
1-11: 型定義が明確です
InfoFeatureFlags型は9つの機能フラグを非オプショナルな boolean として定義しており、型安全性を確保しています。基本機能とゲーム機能の両方を網羅しています。
13-15: ヘルパー関数の実装が適切です
isEnabledByDefaultUnlessFalseは、未定義の値をデフォルトで有効として扱う動作を正しく実装しています。レガシー設定との後方互換性を保ちながら、明示的な無効化も可能にします。
17-39: 機能フラグ解決ロジックが正確に実装されています
resolveInfoFeatureFlags関数は、オプトイン機能とデフォルト有効な機能を正しく区別しています:
- オプトイン機能（keywordSearch、reversi等）: 明示的に
trueの場合のみ有効- デフォルト有効な機能（autoWord、chart、timeSignal）: 明示的に
falseでない限り有効この実装により、issue #306 で報告された機能情報の不整合が解消されます。
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/config-updater.ts (2)
29-44: エラーメッセージでハードコードされたファイル名の使用カスタム
templatePathを指定した場合でも、エラーメッセージは常にexample.config.tomlを参照しています。実際のパスを使用することで、デバッグが容易になります。🔎 提案される修正
function loadConfigTemplate(options?: StartupConfigCheckOptions): Config { const templatePath = resolvePathFromOptions(options, 'template'); if (!existsSync(templatePath)) { throw new Error( - 'example.config.toml が見つかりません。テンプレートファイルが必要です。' + `${templatePath} が見つかりません。テンプレートファイルが必要です。` ); } try { const templateData = readFileSync(templatePath, 'utf8'); return TOML.parse(templateData) as Config; } catch (error) { - throw new Error(`example.config.toml の読み込みに失敗しました: ${error}`); + throw new Error(`${templatePath} の読み込みに失敗しました: ${error}`); } }
135-155: 冗長な条件チェックLine 140 の
if (missingKeys.length > 0)は、Line 135-138 でlength === 0の場合に既に return しているため、常に true になります。この条件は削除できます。🔎 提案される修正
if (missingKeys.length === 0) { logger.log('✅ 設定ファイルは完全です'); return userConfig; } - if (missingKeys.length > 0) { - logger.log( - `\n📋 以下の設定項目が不足しています (${missingKeys.length}個):` - ); + logger.log( + `\n📋 以下の設定項目が不足しています (${missingKeys.length}個):` + ); - logger.log('━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'); + logger.log('━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'); - missingKeys.forEach((key) => { - const defaultValue = getNestedValue(template, key); - const valuePreview = formatValuePreview(defaultValue); - logger.log(` 📝 ${key} = ${valuePreview}`); - }); + missingKeys.forEach((key) => { + const defaultValue = getNestedValue(template, key); + const valuePreview = formatValuePreview(defaultValue); + logger.log(` 📝 ${key} = ${valuePreview}`); + }); - logger.log('━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'); - logger.log('💡 これらの設定を config.toml に追加することをお勧めします'); - logger.log('📖 詳細は example.config.toml を参照してください'); - } + logger.log('━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━'); + logger.log('💡 これらの設定を config.toml に追加することをお勧めします'); + logger.log('📖 詳細は example.config.toml を参照してください');test/config-updater.test.mjs (1)
33-73: テストロジックは正しいですが、一時ディレクトリのクリーンアップを検討してくださいテストケースは不足しているキー（top-level と nested）の検出を正しく検証しています。ただし、
mkdtempで作成された一時ディレクトリはテスト終了後にクリーンアップされません。CI環境でのディスク使用量を抑えるため、test.afterやt.afterでクリーンアップすることを検討してください。🔎 クリーンアップの例
import { rm } from 'node:fs/promises'; test('checkMissingConfigKeys: reports missing keys', async (t) => { const dir = await mkdtemp(path.join(os.tmpdir(), 'ai-config-test-')); t.after(() => rm(dir, { recursive: true, force: true })); // ... rest of test });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/config-updater.ts(3 hunks)test/config-updater.test.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/config-updater.test.mjs (1)
src/config-updater.ts (2)
checkMissingConfigKeys(121-163)performStartupConfigCheck(168-203)
src/config-updater.ts (1)
test/config-updater.test.mjs (9)
templatePath(35-35)templatePath(77-77)templatePath(156-156)userConfig(55-59)userConfig(93-97)userConfig(107-107)error(27-27)configPath(135-135)configPath(155-155)
🔇 Additional comments (6)
src/config-updater.ts (2)
10-27: LGTM! テスト可能な設計への良いリファクタリング依存性注入パターンを使用して、
consoleとprocess.exitへの直接依存を排除しています。ConfigCheckLoggerとStartupConfigCheckOptionsの型定義により、テスト時にモック可能な柔軟な設計になっています。
168-202: LGTM! テスト可能な起動チェック関数
performStartupConfigCheckは依存性注入を正しく活用しており、exitハンドラーを注入可能にすることでテスト時に例外をスローするモックを使用できます。エラーハンドリングも適切に実装されています。test/config-updater.test.mjs (4)
12-31: LGTM! テストヘルパー関数の良い設計
createCaptureLoggerとexitThatThrowsは依存性注入パターンをテストするための適切なヘルパーです。exitThatThrowsがcodeプロパティ付きの例外をスローすることで、exit コードの検証が可能になっています。
75-102: LGTM!設定が完全な場合の成功メッセージを正しく検証しています。
104-116: LGTM!テンプレートファイルが見つからない場合のエラーハンドリングとグレースフルデグラデーション（
userConfigを返す）を正しく検証しています。
118-172: LGTM! performStartupConfigCheck のテストカバレッジが良好以下のシナリオが適切にテストされています：
config.tomlが存在しない場合の exit(1)- 無効な TOML の場合の exit(1)
- 成功時の設定オブジェクト返却
assert.throwsのカスタム述語を使用してエラーコードを検証する手法も適切です。
…with tests
resolve: #306
Summary by CodeRabbit
新機能
リファクタ
テスト
その他
✏️ Tip: You can customize this high-level summary in your review settings.