-
Notifications
You must be signed in to change notification settings - Fork 71
Claude/analyze test coverage dkq44 #119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
tobimeira
wants to merge
5
commits into
firebase:main
Choose a base branch
from
tobimeira:claude/analyze-test-coverage-DKQ44
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
12a29e8
Update Claude plugin marketplace command to firebase/skills
charlotteliang 1dba400
Merge remote-tracking branch 'origin/update-claude-plugin-readme'
tobimeira f01b11f
Add automated test suite for JS utilities, shell scripts, and Markdow…
claude ec7d16d
Guard countTokens against uninitialized model
claude b551c63
Add project allowlist for read-only GitHub MCP tool
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| #!/bin/bash | ||
| # Shared test helpers and assertion functions. | ||
| # Counts are stored in temp files so subshells can contribute to the total. | ||
|
|
||
| _PASS_FILE="" | ||
| _FAIL_FILE="" | ||
|
|
||
| init_counters() { | ||
| _PASS_FILE=$(mktemp) | ||
| _FAIL_FILE=$(mktemp) | ||
| echo 0 > "$_PASS_FILE" | ||
| echo 0 > "$_FAIL_FILE" | ||
| export _PASS_FILE _FAIL_FILE | ||
| } | ||
|
|
||
| _inc_pass() { echo $(($(cat "$_PASS_FILE") + 1)) > "$_PASS_FILE"; } | ||
| _inc_fail() { echo $(($(cat "$_FAIL_FILE") + 1)) > "$_FAIL_FILE"; } | ||
|
|
||
| pass() { | ||
| echo " ✓ $1" | ||
| _inc_pass | ||
| } | ||
|
|
||
| fail() { | ||
| local name="$1" | ||
| local reason="${2:-}" | ||
| echo " ✗ $name" | ||
| [ -n "$reason" ] && echo " $reason" | ||
| _inc_fail | ||
| } | ||
|
|
||
| assert_equals() { | ||
| local expected="$1" actual="$2" name="$3" | ||
| if [ "$expected" = "$actual" ]; then pass "$name" | ||
| else fail "$name" "expected: '$expected' actual: '$actual'"; fi | ||
| } | ||
|
|
||
| assert_file_exists() { | ||
| local file="$1" name="$2" | ||
| if [ -f "$file" ]; then pass "$name" | ||
| else fail "$name" "file not found: $file"; fi | ||
| } | ||
|
|
||
| assert_dir_exists() { | ||
| local dir="$1" name="$2" | ||
| if [ -d "$dir" ]; then pass "$name" | ||
| else fail "$name" "directory not found: $dir"; fi | ||
| } | ||
|
|
||
| assert_not_exists() { | ||
| local path="$1" name="$2" | ||
| if [ ! -e "$path" ]; then pass "$name" | ||
| else fail "$name" "expected path to not exist: $path"; fi | ||
| } | ||
|
|
||
| assert_file_contains() { | ||
| local file="$1" pattern="$2" name="$3" | ||
| if grep -q "$pattern" "$file" 2>/dev/null; then pass "$name" | ||
| else fail "$name" "pattern '$pattern' not found in $file"; fi | ||
| } | ||
|
|
||
| print_summary() { | ||
| local suite="$1" | ||
| local passed failed | ||
| passed=$(cat "$_PASS_FILE") | ||
| failed=$(cat "$_FAIL_FILE") | ||
| rm -f "$_PASS_FILE" "$_FAIL_FILE" | ||
| echo "" | ||
| echo "--- $suite ---" | ||
| echo " Passed: $passed Failed: $failed" | ||
| [ "$failed" -eq 0 ] | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| #!/bin/bash | ||
| # Entry point: runs all shell script test suites | ||
| set -e | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
|
|
||
| OVERALL_FAILED=0 | ||
|
|
||
| run_suite() { | ||
| local suite_script="$1" | ||
| bash "$suite_script" | ||
| local exit_code=$? | ||
| if [ $exit_code -ne 0 ]; then | ||
| OVERALL_FAILED=$((OVERALL_FAILED + 1)) | ||
| fi | ||
| } | ||
|
|
||
| echo "========================================" | ||
| echo " Shell Script Test Suite" | ||
| echo "========================================" | ||
| echo "" | ||
|
|
||
| run_suite "$SCRIPT_DIR/test_sync_skills.sh" | ||
| echo "" | ||
| run_suite "$SCRIPT_DIR/test_prune_skills.sh" | ||
|
|
||
| echo "" | ||
| echo "========================================" | ||
| if [ "$OVERALL_FAILED" -gt 0 ]; then | ||
| echo " RESULT: $OVERALL_FAILED suite(s) FAILED" | ||
| echo "========================================" | ||
| exit 1 | ||
| else | ||
| echo " RESULT: All suites passed" | ||
| echo "========================================" | ||
| fi |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| #!/bin/bash | ||
| # Tests for prune-skills.sh | ||
| set -e | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| SCRIPTS_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" | ||
| # shellcheck source=helpers.sh | ||
| source "$SCRIPT_DIR/helpers.sh" | ||
|
|
||
| # Writes a mock yq that returns "true"/"false" based on file content | ||
| make_mock_yq() { | ||
| local bin_dir="$1" | ||
| cat > "$bin_dir/yq" << 'EOF' | ||
| #!/bin/bash | ||
| if [ "$1" = "-f" ] && [ "$2" = "extract" ]; then | ||
| file="$4" | ||
| if grep -q "genkit-managed: true" "$file" 2>/dev/null; then | ||
| echo "true" | ||
| else | ||
| echo "false" | ||
| fi | ||
| fi | ||
| EOF | ||
| chmod +x "$bin_dir/yq" | ||
| } | ||
|
|
||
| init_counters | ||
| echo "Running prune-skills.sh tests..." | ||
|
|
||
| # ── Test 1: genkit-managed skill absent from source is deleted ──────────────── | ||
| ( | ||
| tmpdir=$(mktemp -d) | ||
| make_mock_yq "$tmpdir" | ||
| cd "$tmpdir" | ||
| export PATH="$tmpdir:$PATH" | ||
| mkdir -p firebase-skills/skills/deleted-skill | ||
| printf -- '---\nmetadata:\n genkit-managed: true\n---\n' > firebase-skills/skills/deleted-skill/SKILL.md | ||
| mkdir -p genkit-skills/skills | ||
| bash "$SCRIPTS_DIR/prune-skills.sh" > /dev/null 2>&1 | ||
| assert_not_exists "$tmpdir/firebase-skills/skills/deleted-skill" "orphaned genkit-managed skill is pruned" | ||
| rm -rf "$tmpdir" | ||
| ) | ||
|
|
||
| # ── Test 2: genkit-managed skill present in source is kept ─────────────────── | ||
| ( | ||
| tmpdir=$(mktemp -d) | ||
| make_mock_yq "$tmpdir" | ||
| cd "$tmpdir" | ||
| export PATH="$tmpdir:$PATH" | ||
| mkdir -p firebase-skills/skills/kept-skill | ||
| printf -- '---\nmetadata:\n genkit-managed: true\n---\n' > firebase-skills/skills/kept-skill/SKILL.md | ||
| mkdir -p genkit-skills/skills/kept-skill | ||
| bash "$SCRIPTS_DIR/prune-skills.sh" > /dev/null 2>&1 | ||
| assert_dir_exists "$tmpdir/firebase-skills/skills/kept-skill" "active genkit-managed skill is kept" | ||
| rm -rf "$tmpdir" | ||
| ) | ||
|
|
||
| # ── Test 3: non-genkit-managed skill is never pruned ───────────────────────── | ||
| ( | ||
| tmpdir=$(mktemp -d) | ||
| make_mock_yq "$tmpdir" | ||
| cd "$tmpdir" | ||
| export PATH="$tmpdir:$PATH" | ||
| mkdir -p firebase-skills/skills/local-skill | ||
| printf -- '---\nname: local-skill\n---\n' > firebase-skills/skills/local-skill/SKILL.md | ||
| mkdir -p genkit-skills/skills | ||
| bash "$SCRIPTS_DIR/prune-skills.sh" > /dev/null 2>&1 | ||
| assert_dir_exists "$tmpdir/firebase-skills/skills/local-skill" "non-managed skill is never pruned" | ||
| rm -rf "$tmpdir" | ||
| ) | ||
|
|
||
| # ── Test 4: skill directory without SKILL.md is left untouched ─────────────── | ||
| ( | ||
| tmpdir=$(mktemp -d) | ||
| make_mock_yq "$tmpdir" | ||
| cd "$tmpdir" | ||
| export PATH="$tmpdir:$PATH" | ||
| mkdir -p firebase-skills/skills/no-skill-md | ||
| mkdir -p genkit-skills/skills | ||
| bash "$SCRIPTS_DIR/prune-skills.sh" > /dev/null 2>&1 | ||
| assert_dir_exists "$tmpdir/firebase-skills/skills/no-skill-md" "directory without SKILL.md is left alone" | ||
| rm -rf "$tmpdir" | ||
| ) | ||
|
|
||
| # ── Test 5: multiple orphaned managed skills are all pruned ────────────────── | ||
| ( | ||
| tmpdir=$(mktemp -d) | ||
| make_mock_yq "$tmpdir" | ||
| cd "$tmpdir" | ||
| export PATH="$tmpdir:$PATH" | ||
| for skill in orphan-a orphan-b; do | ||
| mkdir -p "firebase-skills/skills/$skill" | ||
| printf -- '---\nmetadata:\n genkit-managed: true\n---\n' > "firebase-skills/skills/$skill/SKILL.md" | ||
| done | ||
| mkdir -p genkit-skills/skills | ||
| bash "$SCRIPTS_DIR/prune-skills.sh" > /dev/null 2>&1 | ||
| assert_not_exists "$tmpdir/firebase-skills/skills/orphan-a" "first orphaned skill is pruned" | ||
| assert_not_exists "$tmpdir/firebase-skills/skills/orphan-b" "second orphaned skill is pruned" | ||
| rm -rf "$tmpdir" | ||
| ) | ||
|
|
||
| # ── Test 6: only the managed orphan is pruned when mixed with local skills ──── | ||
| ( | ||
| tmpdir=$(mktemp -d) | ||
| make_mock_yq "$tmpdir" | ||
| cd "$tmpdir" | ||
| export PATH="$tmpdir:$PATH" | ||
| mkdir -p firebase-skills/skills/managed-orphan | ||
| printf -- '---\nmetadata:\n genkit-managed: true\n---\n' > firebase-skills/skills/managed-orphan/SKILL.md | ||
| mkdir -p firebase-skills/skills/local-skill | ||
| printf -- '---\nname: local\n---\n' > firebase-skills/skills/local-skill/SKILL.md | ||
| mkdir -p genkit-skills/skills | ||
| bash "$SCRIPTS_DIR/prune-skills.sh" > /dev/null 2>&1 | ||
| assert_not_exists "$tmpdir/firebase-skills/skills/managed-orphan" "managed orphan is pruned" | ||
| assert_dir_exists "$tmpdir/firebase-skills/skills/local-skill" "unmanaged skill is preserved" | ||
| rm -rf "$tmpdir" | ||
| ) | ||
|
|
||
| print_summary "prune-skills.sh" | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| #!/bin/bash | ||
| # Tests for sync-skills.sh | ||
| set -e | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| SCRIPTS_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" | ||
| # shellcheck source=helpers.sh | ||
| source "$SCRIPT_DIR/helpers.sh" | ||
|
|
||
| # Writes a mock yq binary into the given directory | ||
| make_mock_yq() { | ||
| local bin_dir="$1" | ||
| cat > "$bin_dir/yq" << 'EOF' | ||
| #!/bin/bash | ||
| # Mock yq: handles -i -f process '...' <file> | ||
| if [ "$1" = "-i" ] && [ "$2" = "-f" ] && [ "$3" = "process" ]; then | ||
| shift 4 | ||
| file="$1" | ||
| printf '\ngenkit-managed: true\n' >> "$file" | ||
| fi | ||
| EOF | ||
| chmod +x "$bin_dir/yq" | ||
| } | ||
|
|
||
| init_counters | ||
| echo "Running sync-skills.sh tests..." | ||
|
|
||
| # ── Test 1: skill directory is created at destination ───────────────────────── | ||
| ( | ||
| tmpdir=$(mktemp -d) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| make_mock_yq "$tmpdir" | ||
| cd "$tmpdir" | ||
| export PATH="$tmpdir:$PATH" | ||
| mkdir -p genkit-skills/skills/my-skill | ||
| echo "name: my-skill" > genkit-skills/skills/my-skill/SKILL.md | ||
| mkdir -p firebase-skills/skills | ||
| bash "$SCRIPTS_DIR/sync-skills.sh" > /dev/null 2>&1 | ||
| assert_dir_exists "$tmpdir/firebase-skills/skills/my-skill" "destination directory is created" | ||
| rm -rf "$tmpdir" | ||
| ) | ||
|
|
||
| # ── Test 2: SKILL.md is copied to destination ───────────────────────────────── | ||
| ( | ||
| tmpdir=$(mktemp -d) | ||
| make_mock_yq "$tmpdir" | ||
| cd "$tmpdir" | ||
| export PATH="$tmpdir:$PATH" | ||
| mkdir -p genkit-skills/skills/my-skill | ||
| echo "name: my-skill" > genkit-skills/skills/my-skill/SKILL.md | ||
| mkdir -p firebase-skills/skills | ||
| bash "$SCRIPTS_DIR/sync-skills.sh" > /dev/null 2>&1 | ||
| assert_file_exists "$tmpdir/firebase-skills/skills/my-skill/SKILL.md" "SKILL.md is copied" | ||
| rm -rf "$tmpdir" | ||
| ) | ||
|
|
||
| # ── Test 3: reference files are copied alongside SKILL.md ──────────────────── | ||
| ( | ||
| tmpdir=$(mktemp -d) | ||
| make_mock_yq "$tmpdir" | ||
| cd "$tmpdir" | ||
| export PATH="$tmpdir:$PATH" | ||
| mkdir -p genkit-skills/skills/my-skill/references | ||
| echo "content" > genkit-skills/skills/my-skill/SKILL.md | ||
| echo "ref content" > genkit-skills/skills/my-skill/references/guide.md | ||
| mkdir -p firebase-skills/skills | ||
| bash "$SCRIPTS_DIR/sync-skills.sh" > /dev/null 2>&1 | ||
| assert_file_exists "$tmpdir/firebase-skills/skills/my-skill/references/guide.md" "reference files are copied" | ||
| rm -rf "$tmpdir" | ||
| ) | ||
|
|
||
| # ── Test 4: yq is called to tag the skill as genkit-managed ────────────────── | ||
| ( | ||
| tmpdir=$(mktemp -d) | ||
| make_mock_yq "$tmpdir" | ||
| cd "$tmpdir" | ||
| export PATH="$tmpdir:$PATH" | ||
| mkdir -p genkit-skills/skills/my-skill | ||
| echo "name: my-skill" > genkit-skills/skills/my-skill/SKILL.md | ||
| mkdir -p firebase-skills/skills | ||
| bash "$SCRIPTS_DIR/sync-skills.sh" > /dev/null 2>&1 | ||
| assert_file_contains "$tmpdir/firebase-skills/skills/my-skill/SKILL.md" "genkit-managed" "yq tags skill as genkit-managed" | ||
| rm -rf "$tmpdir" | ||
| ) | ||
|
|
||
| # ── Test 5: stale files at destination are removed before sync ─────────────── | ||
| ( | ||
| tmpdir=$(mktemp -d) | ||
| make_mock_yq "$tmpdir" | ||
| cd "$tmpdir" | ||
| export PATH="$tmpdir:$PATH" | ||
| mkdir -p genkit-skills/skills/my-skill | ||
| echo "content" > genkit-skills/skills/my-skill/SKILL.md | ||
| mkdir -p firebase-skills/skills/my-skill | ||
| echo "stale" > firebase-skills/skills/my-skill/old-file.md | ||
| bash "$SCRIPTS_DIR/sync-skills.sh" > /dev/null 2>&1 | ||
| assert_not_exists "$tmpdir/firebase-skills/skills/my-skill/old-file.md" "stale destination files are removed" | ||
| rm -rf "$tmpdir" | ||
| ) | ||
|
|
||
| # ── Test 6: multiple skills are all synced ─────────────────────────────────── | ||
| ( | ||
| tmpdir=$(mktemp -d) | ||
| make_mock_yq "$tmpdir" | ||
| cd "$tmpdir" | ||
| export PATH="$tmpdir:$PATH" | ||
| for skill in skill-a skill-b skill-c; do | ||
| mkdir -p "genkit-skills/skills/$skill" | ||
| echo "name: $skill" > "genkit-skills/skills/$skill/SKILL.md" | ||
| done | ||
| mkdir -p firebase-skills/skills | ||
| bash "$SCRIPTS_DIR/sync-skills.sh" > /dev/null 2>&1 | ||
| assert_dir_exists "$tmpdir/firebase-skills/skills/skill-a" "first skill synced" | ||
| assert_dir_exists "$tmpdir/firebase-skills/skills/skill-b" "second skill synced" | ||
| assert_dir_exists "$tmpdir/firebase-skills/skills/skill-c" "third skill synced" | ||
| rm -rf "$tmpdir" | ||
| ) | ||
|
|
||
| print_summary "sync-skills.sh" | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| { | ||
| "default": true, | ||
| "MD013": false, | ||
| "MD033": false, | ||
| "MD041": false, | ||
| "MD024": { "siblings_only": true }, | ||
| "MD007": { "indent": 2 }, | ||
| "MD012": { "maximum": 2 } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| { | ||
| "name": "agent-skills", | ||
| "version": "1.0.0", | ||
| "private": true, | ||
| "scripts": { | ||
| "test": "npm run test:js && npm run test:shell", | ||
| "test:js": "npm test --prefix scripts/skill-token-counter", | ||
| "test:shell": "bash .github/scripts/tests/run_tests.sh", | ||
| "lint:md": "markdownlint '**/*.md' --ignore node_modules --ignore scripts/skill-token-counter/node_modules" | ||
| }, | ||
| "devDependencies": { | ||
| "markdownlint-cli": "^0.43.0" | ||
| } | ||
| } |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The temporary directory created with
mktemp -dis only cleaned up at the end of the subshell. If any command fails before reaching therm -rf(due toset -e), the directory will leak. It is safer to use atrapfor cleanup immediately after creation, which also allows removing the explicitrm -rfat the end of the block.