Skip to content

Refactor test infrastructure and fix GitHub Actions workflow issues#76

Merged
PhantomDave merged 3 commits intoadded-test-workflowfrom
copilot/sub-pr-75
Nov 17, 2025
Merged

Refactor test infrastructure and fix GitHub Actions workflow issues#76
PhantomDave merged 3 commits intoadded-test-workflowfrom
copilot/sub-pr-75

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Nov 17, 2025

Addressed code review feedback on test infrastructure and GitHub Actions workflow. The workflow used outdated action versions, lacked backend setup for E2E tests, and test code had maintainability issues.

Test Code Quality

  • Simplified AccountServiceTests password hashing helper to use PBKDF2 directly instead of creating temporary service instances with complex mocks
  • Extracted duplicate GraphQL queries in AccountIntegrationTests to reusable helper methods (CreateAccountMutation, LoginAccountMutation)
  • Added null-safe JSON parsing with explicit error messages instead of direct property access that could throw cryptic exceptions
  • Removed unused _factory field

Test Isolation

  • Fixed race conditions in parallel test execution by using unique in-memory database names per test instance: $"InMemoryTestDb_{Guid.NewGuid()}"
  • Added verification of account creation before testing dependent operations (login, duplicate email)

GitHub Actions Workflow

  • Updated action versions: checkout@v5, setup-dotnet@v5, setup-node@v6
  • Added npm caching via cache: 'npm'
  • Added database and API startup with health checks before E2E tests (E2E requires backend for GraphQL codegen)
  • Downgraded .NET target from 10.0.x to 9.0.x (10.0 not yet available in GitHub Actions)

Documentation & Build

  • Fixed test counts in TEST_SUMMARY.md (5 JWT, 12 Account = 17 total)
  • Removed x64/x86 platform configurations from solution file (unnecessary for .NET)
  • Optimized run-all-tests.sh to eliminate duplicate test executions and parse counts dynamically
  • Made Playwright baseURL configurable via environment variable
// Before: hardcoded
baseURL: 'http://localhost:4200',

// After: configurable
baseURL: process.env.BASE_URL || 'http://localhost:4200',

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 2 commits November 17, 2025 11:44
Co-authored-by: PhantomDave <34485699+PhantomDave@users.noreply.github.com>
…nd setup for E2E tests

Co-authored-by: PhantomDave <34485699+PhantomDave@users.noreply.github.com>
Copilot AI changed the title [WIP] Add GitHub Actions workflow for testing .NET and frontend applications Refactor test infrastructure and fix GitHub Actions workflow issues Nov 17, 2025
Copilot AI requested a review from PhantomDave November 17, 2025 11:49
@PhantomDave PhantomDave marked this pull request as ready for review November 17, 2025 11:53
Copilot AI review requested due to automatic review settings November 17, 2025 11:53
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors test infrastructure and fixes GitHub Actions workflow configuration. It addresses code review feedback by simplifying test helpers, improving test isolation, updating outdated action versions, and adding proper backend/database setup for E2E tests.

Key Changes

  • Simplified password hashing in unit tests by using PBKDF2 directly instead of creating temporary service instances
  • Fixed parallel test race conditions with unique in-memory database names per test instance
  • Updated GitHub Actions to use latest action versions (v5/v6) and added backend/database startup with health checks

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
PhantomDave.BankTracking.UnitTests/Services/AccountServiceTests.cs Refactored password hashing helper to be static and use PBKDF2 directly, eliminating complex mock setup
PhantomDave.BankTracking.IntegrationTests/GraphQL/AccountIntegrationTests.cs Extracted duplicate GraphQL queries to helper methods and added safe JSON parsing with explicit error messages
PhantomDave.BankTracking.IntegrationTests/Helpers/GraphQLTestFactory.cs Fixed race conditions by using unique GUID-based in-memory database names per test instance
run-all-tests.sh Optimized script to eliminate duplicate test executions and dynamically parse test counts from output
frontend/playwright.config.ts Made Playwright baseURL configurable via environment variable for flexibility in different environments
frontend/e2e/app.spec.ts Added explicit timeout to password input visibility assertion for consistency
TEST_SUMMARY.md Corrected test counts (5 JWT + 12 Account = 17 total) and reformatted test list structure
PhantomDave.BankTracking.sln Removed unnecessary x64/x86 platform configurations, keeping only Any CPU
.github/workflows/tests.yml Updated to latest action versions, added database and API startup with health checks for E2E tests, downgraded .NET to 9.0.x

Comment on lines +41 to +44
- name: Start Backend API
run: |
dotnet build PhantomDave.BankTracking.Api/
nohup dotnet run --project PhantomDave.BankTracking.Api/ --urls=http://localhost:5095 > api.log 2>&1 &
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

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

Consider adding a step to output API logs on failure for debugging. The API is started with output redirected to api.log, but if E2E tests fail, the logs aren't automatically displayed. Add a cleanup step with if: failure() that runs cat api.log to help diagnose issues.

Copilot uses AI. Check for mistakes.
Comment on lines +74 to +77
UNIT_COUNT=$(echo "$UNIT_OUTPUT" | grep -oP 'Total:\s*\K\d+' || echo "?")
UNIT_PASSED=$(echo "$UNIT_OUTPUT" | grep -oP 'Passed:\s*\K\d+' || echo "?")
INT_COUNT=$(echo "$INT_OUTPUT" | grep -oP 'Total:\s*\K\d+' || echo "?")
INT_PASSED=$(echo "$INT_OUTPUT" | grep -oP 'Passed:\s*\K\d+' || echo "?")
Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

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

Grep pattern may not match actual dotnet test output format. The pattern 'Total:\s*\K\d+' uses Perl-compatible regex (\K lookbehind) with grep -oP, but dotnet test output typically uses format like "Total: 17" or "Passed: 17". Verify this pattern works with actual output, or replace with a more compatible pattern like grep -oE 'Total: [0-9]+' | grep -oE '[0-9]+' for better portability.

Suggested change
UNIT_COUNT=$(echo "$UNIT_OUTPUT" | grep -oP 'Total:\s*\K\d+' || echo "?")
UNIT_PASSED=$(echo "$UNIT_OUTPUT" | grep -oP 'Passed:\s*\K\d+' || echo "?")
INT_COUNT=$(echo "$INT_OUTPUT" | grep -oP 'Total:\s*\K\d+' || echo "?")
INT_PASSED=$(echo "$INT_OUTPUT" | grep -oP 'Passed:\s*\K\d+' || echo "?")
UNIT_COUNT=$(echo "$UNIT_OUTPUT" | grep -oE 'Total: [0-9]+' | grep -oE '[0-9]+' || echo "?")
UNIT_PASSED=$(echo "$UNIT_OUTPUT" | grep -oE 'Passed: [0-9]+' | grep -oE '[0-9]+' || echo "?")
INT_COUNT=$(echo "$INT_OUTPUT" | grep -oE 'Total: [0-9]+' | grep -oE '[0-9]+' || echo "?")
INT_PASSED=$(echo "$INT_OUTPUT" | grep -oE 'Passed: [0-9]+' | grep -oE '[0-9]+' || echo "?")

Copilot uses AI. Check for mistakes.

- name: Start Database
run: docker compose -f compose.dev.yaml up -d database

Copy link

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

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

Missing database health check. The database is started but there's no verification that it's ready before starting the API. The API startup could fail or encounter errors if it attempts to connect before PostgreSQL is fully initialized. Add a wait step similar to lines 46-57 to verify the database is ready (e.g., docker compose -f compose.dev.yaml exec database pg_isready).

Suggested change
- name: Wait for Database
run: |
for i in {1..30}; do
if docker compose -f compose.dev.yaml exec -T database pg_isready -U postgres; then
echo "Database is ready!"
exit 0
fi
echo "Waiting for database..."
sleep 2
done
echo "Database did not become ready in time" >&2
exit 1

Copilot uses AI. Check for mistakes.
@PhantomDave PhantomDave merged commit ef42901 into added-test-workflow Nov 17, 2025
10 of 13 checks passed
@PhantomDave PhantomDave deleted the copilot/sub-pr-75 branch November 17, 2025 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants