Skip to content

🧪 Add comprehensive test suite for shell scripts (#73) #78

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.

Sign up for GitHub

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

Closed
keito4 wants to merge 1 commit into from
Closed

🧪 Add comprehensive test suite for shell scripts (#73) #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
269 changes: 264 additions & 5 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,289 @@ on:
push:
branches: [main, master]

# Cancel in-progress runs for same PR/branch
concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true

jobs:
quality:
# Dependency installation with caching
setup:
runs-on: ubuntu-latest
outputs:
cache-key: ${{ steps.cache-keys.outputs.npm }}
steps:
- uses: actions/checkout@v4

- name: Generate cache keys
id: cache-keys
run: |
echo "npm=${{ runner.os }}-npm-${{ hashFiles('**/package-lock.json') }}" >> $GITHUB_OUTPUT

- uses: actions/setup-node@v4
with:
node-version: '20'
cache: 'npm'

- name: Cache npm dependencies
uses: actions/cache@v4
id: npm-cache
with:
path: |
~/.npm
node_modules
key: ${{ steps.cache-keys.outputs.npm }}
restore-keys: |
${{ runner.os }}-npm-

- name: Install dependencies
if: steps.npm-cache.outputs.cache-hit != 'true'
run: npm ci

- name: Upload node_modules
uses: actions/upload-artifact@v4
with:
name: node-modules
path: node_modules
retention-days: 1

# Parallel linting job
lint:
needs: setup
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

- uses: actions/setup-node@v4
with:
node-version: '20'

- name: Download node_modules
uses: actions/download-artifact@v4
with:
name: node-modules
path: node_modules

- name: Run linter
run: npm run lint

- name: Upload lint results
if: always()
uses: actions/upload-artifact@v4
with:
name: lint-results
path: |
**/eslint-report.json
**/eslint-report.html
retention-days: 7

# Parallel formatting check
format:
needs: setup
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

- uses: actions/setup-node@v4
with:
node-version: '20'

- name: Download node_modules
uses: actions/download-artifact@v4
with:
name: node-modules
path: node_modules

- name: Check formatting
run: npm run format:check

- name: Run tests
run: npm run test
continue-on-error: true # テストが未設定のため一時的にエラーを無視
# Parallel unit tests with coverage
unit-tests:
needs: setup
runs-on: ubuntu-latest
strategy:
matrix:
shard: [1, 2, 3, 4]
steps:
- uses: actions/checkout@v4

- uses: actions/setup-node@v4
with:
node-version: '20'

- name: Download node_modules
uses: actions/download-artifact@v4
with:
name: node-modules
path: node_modules

- name: Run unit tests (shard ${{ matrix.shard }}/4)
run: |
npm run test -- --shard=${{ matrix.shard }}/4 --coverage --coverageReporters=json --coverageReporters=lcov --coverageReporters=text --coverageReporters=cobertura
continue-on-error: true
Comment on lines +124 to +127

@coderabbitai coderabbitai Bot Aug 4, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

unit-tests job invokes shell runner with Jest-style flags – will always report zero coverage

npm run test -- --shard=… --coverage … now resolves to ./run-tests.sh, which ignores these flags and produces no coverage/ directory.
Downstream “merge coverage” and threshold checks will fail.

Recommended fix:

-          npm run test -- --shard=${{ matrix.shard }}/4 --coverage ...
+          npm run test:node -- --shard=${{ matrix.shard }}/4 --coverage ...

and ensure test:node actually runs the JS unit-test framework.

📝 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
- name: Run unit tests (shard ${{ matrix.shard }}/4)
run: |
npm run test -- --shard=${{ matrix.shard }}/4 --coverage --coverageReporters=json --coverageReporters=lcov --coverageReporters=text --coverageReporters=cobertura
continue-on-error: true
- name: Run unit tests (shard ${{ matrix.shard }}/4)
run: |
npm run test:node -- --shard=${{ matrix.shard }}/4 --coverage --coverageReporters=json --coverageReporters=lcov --coverageReporters=text --coverageReporters=cobertura
continue-on-error: true
🤖 Prompt for AI Agents 
In .github/workflows/ci.yml lines 124 to 127, the unit-tests job runs `npm run
test` with Jest flags, but this triggers a shell script that ignores these flags
and does not generate coverage data. To fix this, modify the workflow to run the
JavaScript unit test framework directly (e.g., `npm run test:node`) instead of
`npm run test`, ensuring the test command accepts Jest flags and produces the
coverage directory needed for downstream steps.


Comment on lines +124 to +128

@coderabbitai coderabbitai Bot Aug 4, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

continue-on-error: true hides failing unit tests

Because the unit-tests job continues on error, a broken test suite will still report success, and subsequent jobs will proceed. The quality gate only inspects coverage, not test success, so regressions can be merged unnoticed.

Remove the flag or gate on ${{ steps.<test-step>.outcome }} explicitly.

🤖 Prompt for AI Agents 
In .github/workflows/ci.yml at lines 124 to 128, the unit test step uses
'continue-on-error: true', which causes failing tests to be ignored and the job
to report success. Remove the 'continue-on-error: true' line to ensure the
workflow fails on test failures, or alternatively, add a conditional check on
the test step's outcome in subsequent jobs to gate progress based on test
success.

- name: Upload coverage reports
uses: actions/upload-artifact@v4
with:
name: coverage-${{ matrix.shard }}
path: |
coverage/lcov.info
coverage/coverage-final.json
coverage/cobertura-coverage.xml
retention-days: 1

# Merge coverage reports
coverage:
needs: unit-tests
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

- uses: actions/setup-node@v4
with:
node-version: '20'

- name: Download all coverage reports
uses: actions/download-artifact@v4
with:
pattern: coverage-*
path: coverage-reports

- name: Merge coverage reports
run: |
npm install -g nyc
nyc merge coverage-reports coverage/coverage-final.json
nyc report --reporter=lcov --reporter=text --reporter=cobertura

- name: Upload to Codecov
uses: codecov/codecov-action@v4
with:
token: ${{ secrets.CODECOV_TOKEN }}
files: ./coverage/lcov.info
flags: unittests
name: codecov-umbrella
fail_ci_if_error: false

- name: Generate coverage summary
run: |
echo "## Coverage Report" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
nyc report --reporter=text | tail -n +3 >> $GITHUB_STEP_SUMMARY

- name: Check coverage thresholds
run: |
COVERAGE=$(nyc report --reporter=json-summary | jq '.total.lines.pct')
echo "Total coverage: $COVERAGE%"
if (( $(echo "$COVERAGE < 70" | bc -l) )); then
echo "::error::Coverage $COVERAGE% is below 70% threshold"
exit 1
fi
Comment on lines +179 to +184

@coderabbitai coderabbitai Bot Aug 4, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

bc may be missing on the runner

The threshold check uses bc -l, but this package isn’t guaranteed to exist on ubuntu-latest images. Add an explicit install step or replace with POSIX arithmetic (e.g., using awk).

-          if (( $(echo "$COVERAGE < 70" | bc -l) )); then
+          if awk "BEGIN {exit !($COVERAGE < 70)}"; then
             …
📝 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
COVERAGE=$(nyc report --reporter=json-summary | jq '.total.lines.pct')
echo "Total coverage: $COVERAGE%"
if (( $(echo "$COVERAGE < 70" | bc -l) )); then
echo "::error::Coverage $COVERAGE% is below 70% threshold"
exit 1
fi
COVERAGE=$(nyc report --reporter=json-summary | jq '.total.lines.pct')
echo "Total coverage: $COVERAGE%"
if awk "BEGIN {exit !($COVERAGE < 70)}"; then
echo "::error::Coverage $COVERAGE% is below 70% threshold"
exit 1
fi
🤖 Prompt for AI Agents 
In .github/workflows/ci.yml around lines 179 to 184, the script uses `bc -l` for
floating-point comparison, but `bc` may not be installed on the runner. To fix
this, either add a step before this code to explicitly install `bc` (e.g., `sudo
apt-get install -y bc`) or replace the comparison logic with a POSIX-compliant
tool like `awk` to perform the floating-point comparison without relying on
`bc`.


# Shell script tests with parallel execution
shell-tests:
runs-on: ubuntu-latest
strategy:
matrix:
test-suite: [core, utils, integration]
name: Shell Tests - ${{ matrix.test-suite }}
steps:
- uses: actions/checkout@v4

- name: Cache shell test dependencies
uses: actions/cache@v4
with:
path: |
~/.bats
test/libs
key: ${{ runner.os }}-shell-tests-${{ hashFiles('test/setup.sh') }}
restore-keys: |
${{ runner.os }}-shell-tests-

- name: Install test dependencies
run: |
sudo apt-get update
sudo apt-get install -y zsh shellcheck

- name: Setup Bats test framework
run: |
cd test
./setup.sh

- name: Run shell script tests - ${{ matrix.test-suite }}
run: |
cd test
./run-tests.sh --suite=${{ matrix.test-suite }} --verbose --coverage

- name: Upload shell test results
if: always()
uses: actions/upload-artifact@v4
with:
name: shell-test-results-${{ matrix.test-suite }}
path: |
test/results/*.xml
test/coverage/*.info
retention-days: 7

# Build job with caching
build:
needs: setup
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

- uses: actions/setup-node@v4
with:
node-version: '20'

- name: Download node_modules
uses: actions/download-artifact@v4
with:
name: node-modules
path: node_modules

- name: Cache build outputs
uses: actions/cache@v4
with:
path: |
dist
.next
out
key: ${{ runner.os }}-build-${{ github.sha }}
restore-keys: |
${{ runner.os }}-build-

- name: Build
run: npm run build
continue-on-error: true # ビルドプロセスが未設定のため一時的にエラーを無視
continue-on-error: true

- name: Upload build artifacts
uses: actions/upload-artifact@v4
with:
name: build-output
path: |
dist
.next
out
retention-days: 7

# Final quality gate
quality-gate:
needs: [lint, format, coverage, shell-tests, build]
runs-on: ubuntu-latest
if: always()
steps:
- name: Check quality gate status
run: |
if [[ "${{ needs.lint.result }}" != "success" ]]; then
echo "::error::Linting failed"
exit 1
fi
if [[ "${{ needs.format.result }}" != "success" ]]; then
echo "::error::Formatting check failed"
exit 1
fi
if [[ "${{ needs.coverage.result }}" != "success" ]]; then
echo "::error::Coverage requirements not met"
exit 1
fi
echo "✅ All quality gates passed"
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ node_modules/
# Testing
coverage/
.nyc_output/
test/bats-libs/
test/mocks/

# Production
build/
Expand Down
26 changes: 25 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.PHONY: version-patch version-minor version-major version-dry-run credentials clean-credentials list-credentials brew-leaves brew-categorized brew-generate
.PHONY: version-patch version-minor version-major version-dry-run credentials clean-credentials list-credentials brew-leaves brew-categorized brew-generate test test-verbose test-shell test-setup test-coverage

@coderabbitai coderabbitai Bot Aug 4, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add missing test-filter to .PHONY declaration.

The test-filter target is defined but not included in the .PHONY declaration, which could cause unexpected behavior if a file with that name exists.

-.PHONY: version-patch version-minor version-major version-dry-run credentials clean-credentials list-credentials brew-leaves brew-categorized brew-generate test test-verbose test-shell test-setup test-coverage
+.PHONY: version-patch version-minor version-major version-dry-run credentials clean-credentials list-credentials brew-leaves brew-categorized brew-generate test test-verbose test-shell test-setup test-coverage test-filter
🧰 Tools
🪛 checkmake (0.2.2)

[warning] 1-1: Missing required phony target "all"

(minphony)

[warning] 1-1: Missing required phony target "clean"

(minphony)

🤖 Prompt for AI Agents 
In the Makefile at line 1, the .PHONY declaration is missing the test-filter
target. Add test-filter to the list of phony targets in the .PHONY line to
ensure it is treated as a phony target and avoid conflicts with any file named
test-filter.

@coderabbitai coderabbitai Bot Aug 4, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Address missing standard Makefile targets.

The static analysis correctly identifies missing standard targets that are common Makefile conventions.

Add these standard targets:

-.PHONY: version-patch version-minor version-major version-dry-run credentials clean-credentials list-credentials brew-leaves brew-categorized brew-generate test test-verbose test-shell test-setup test-coverage
+.PHONY: all clean version-patch version-minor version-major version-dry-run credentials clean-credentials list-credentials brew-leaves brew-categorized brew-generate test test-verbose test-shell test-setup test-coverage

+# Default target
+all: test ## Run all tests by default
+
+# Clean up generated files
+clean: ## Clean up test artifacts and generated files
+	@echo "Cleaning up test artifacts..."
+	@rm -rf test/bats-libs test/mocks coverage/
📝 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
.PHONY: version-patch version-minor version-major version-dry-run credentials clean-credentials list-credentials brew-leaves brew-categorized brew-generate test test-verbose test-shell test-setup test-coverage
.PHONY: all clean version-patch version-minor version-major version-dry-run credentials clean-credentials list-credentials brew-leaves brew-categorized brew-generate test test-verbose test-shell test-setup test-coverage
# Default target
all: test ## Run all tests by default
# Clean up generated files
clean: ## Clean up test artifacts and generated files
@echo "Cleaning up test artifacts..."
@rm -rf test/bats-libs test/mocks coverage/
🧰 Tools
🪛 checkmake (0.2.2)

[warning] 1-1: Missing required phony target "all"

(minphony)

[warning] 1-1: Missing required phony target "clean"

(minphony)

🤖 Prompt for AI Agents 
In the Makefile at line 1, standard Makefile targets are missing which are
commonly expected for better usability and maintenance. Add standard targets
such as 'all', 'install', 'uninstall', and 'clean' with appropriate commands or
placeholders to ensure the Makefile follows conventional practices and supports
typical build and cleanup operations.


# Semantic versioning for devcontainer
version-patch:
Expand Down Expand Up @@ -43,3 +43,27 @@ brew-deps: ## Show dependencies of a specific package
brew-uses: ## Show packages that depend on a specific package
@if [ -z "$(pkg)" ]; then echo "Usage: make brew-uses pkg=<package>"; exit 1; fi
@./script/brew-deps.sh uses $(pkg)

# Testing
test: test-setup test-shell ## Run all tests

test-setup: ## Setup test framework
@echo "Setting up test framework..."
@cd test && ./setup.sh

test-shell: test-setup ## Run shell script tests
@echo "Running shell script tests..."
@cd test && ./run-tests.sh

test-verbose: test-setup ## Run tests with verbose output
@echo "Running tests with verbose output..."
@cd test && ./run-tests.sh --verbose

test-coverage: test-setup ## Check test coverage
@echo "Checking test coverage..."
@cd test && ./run-tests.sh | grep -q "Coverage meets 70% threshold" && echo "✓ Coverage meets requirements" || (echo "✗ Coverage below 70% threshold" && exit 1)

test-filter: test-setup ## Run specific tests (use with filter=<pattern>)
@if [ -z "$(filter)" ]; then echo "Usage: make test-filter filter=<pattern>"; exit 1; fi
@echo "Running tests matching: $(filter)"
@cd test && ./run-tests.sh --filter "$(filter)"
1 change: 1 addition & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 11 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,16 @@
"scripts": {
"dev": "echo 'No dev server configured'",
"build": "echo 'No build process configured'",
"test": "echo 'No tests configured'",
"test": "cd test && ./run-tests.sh",
"test:shell": "cd test && ./run-tests.sh",
"test:verbose": "cd test && ./run-tests.sh --verbose",
"test:coverage": "cd test && ./run-tests.sh | grep -q 'Coverage meets 70% threshold'",
"test:claude": "node .claude/tests/run-all-tests.js",
Comment on lines +16 to +17

@coderabbitai coderabbitai Bot Aug 4, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Coverage gate via grep is brittle

test:coverage succeeds if grep finds the magic string, regardless of the script’s exit status. A silent failure in run-tests.sh could still return 0 and pass the gate.

Prefer letting run-tests.sh exit non-zero when coverage is below threshold, then simply run it:

-"test:coverage": "cd test && ./run-tests.sh | grep -q 'Coverage meets 70% threshold'",
+"test:coverage": "cd test && ./run-tests.sh --coverage",

and enforce the threshold inside the runner.

📝 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
"test:coverage": "cd test && ./run-tests.sh | grep -q 'Coverage meets 70% threshold'",
"test:claude": "node .claude/tests/run-all-tests.js",
"scripts": {
// …
"test:coverage": "cd test && ./run-tests.sh --coverage",
"test:claude": "node .claude/tests/run-all-tests.js",
// …
}
🤖 Prompt for AI Agents 
In package.json lines 16-17, the test:coverage script uses grep to check for a
coverage string, which can pass even if run-tests.sh fails silently. Modify
run-tests.sh to enforce the coverage threshold internally and exit with a
non-zero status if not met, then update test:coverage to simply run run-tests.sh
without piping to grep, so the script's exit code controls success or failure.

"test:claude:agents": "node .claude/tests/validate-agents.js",
"test:claude:commands": "node .claude/tests/validate-commands.js",
"test:claude:verbose": "node .claude/tests/run-all-tests.js --verbose",
"test:claude:coverage": "node .claude/tests/run-all-tests.js --output",
"validate:claude": "npm run test:claude:agents && npm run test:claude:commands",
"lint": "eslint . --ext .js,.jsx,.ts,.tsx",
"lint:fix": "npm run lint -- --fix",
"format": "prettier --write .",
Expand All @@ -31,6 +40,7 @@
"eslint": "^8.57.1",
"eslint-config-prettier": "^9.1.0",
"husky": "^9.1.7",
"js-yaml": "^4.1.0",
"prettier": "^3.4.2",
"semantic-release": "^24.2.0"
}
Expand Down
Loading
Loading