Add comprehensive test suites, CI workflows, and Copilot instructions#602
Add comprehensive test suites, CI workflows, and Copilot instructions#602
Conversation
- Create Jest test framework for skill activation testing - Implement detection logic based on azure-deploy SKILL.md - Add 104 tests covering: - High confidence signals (azure.yaml, host.json, Dockerfile) - Node.js frameworks (Next.js, Angular, Vue, Express, etc.) - Python frameworks (Flask, Django, FastAPI) - .NET frameworks (ASP.NET Core, Blazor, Azure Functions) - Java frameworks (Spring Boot, Azure Functions) - Static HTML sites - Multi-service architecture detection - Confidence level assignment (HIGH/MEDIUM/LOW) All tests pass: 104/104 ✓
- Add 21 project fixtures with real config files - Add expectations.json mapping fixtures to expected results - Add fixture-loader.js and project-scanner.js utilities - Add service-selection.test.js for fixture iteration - Tests validate: - Service detection (static-web-apps, functions, container-apps, app-service, azd) - Confidence assessment (HIGH/MEDIUM) - Framework detection Total tests: 181 (104 mock-based + 77 fixture-based)
- Setup instructions with prerequisites - Multiple ways to run tests (all, watch, coverage, filtered) - Two options for adding tests (fixtures vs unit tests) - Comprehensive coverage checklist: - Deployment skills coverage table - Framework detection coverage by language - High-confidence signal coverage - Skills not tested (with reasons) - Detection logic reference
- Runs on push/PR to main when plugin/skills or tests change - Tests across Node.js 18, 20, 22 matrix - Uploads coverage report to Codecov (Node 20) - Validates fixtures have matching expectations - Manual trigger via workflow_dispatch
- Create separate test module for Node.js production best practices - Add Express.js validation: trust proxy, health endpoint, cookies, port/host binding - Add Dockerfile validation: NODE_ENV, HEALTHCHECK, non-root user, base image - Include 5 project fixtures with expectations.json - Add independent CI workflow (Node 20, 22 matrix) - 83 tests passing
- Update serviceMapping.js to route all services to azure-deploy - Update expectations.json with new skill name - Update highConfidence.test.js test descriptions - Update README.md with new skill structure and test counts
…reflight skills - Add resourceNameValidator with tests for 8 Azure resource types (Storage Account, Key Vault, ACR, Container App, App Service, etc.) - Add bicepValidator with tests for scope detection, parameter files, security checks - Add preflightValidator with tests for tool detection, auth parsing, reports - Add integration tests for end-to-end scenarios - Add Bicep fixtures and sample project structures 165 tests total, all passing
- Document skill development guidelines and SKILL.md format - Add testing section with guidance to only extend existing suites - Include test update checklist when changing detection/validation logic - Document skill routing and reference guide structure
- Triggers on changes to azure-validation and azure-deployment-preflight skills - Runs on Node.js 20 and 22 - Uploads coverage to Codecov
- Move detection tests from tests/ to tests/detection/ (independent suite) - Rename test.yml to test-detection.yml with updated paths - Update all import paths in test files and utils - Update README.md with new structure - Update copilot-instructions.md with new paths All suites now follow same pattern: - tests/detection/ (181 tests) - tests/nodejs-production/ (83 tests) - tests/validation/ (165 tests)
- Add table of contents and detailed setup instructions - Add 'Run All Suites' command example - Add expected output for each suite - Add detailed test structure with file descriptions - Add complete test coverage tables (frameworks, signals, validation) - Add CI workflows section - Add detection logic reference - Match format from v5 branch with new structure
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive test coverage for Azure deployment skills with three independent test suites totaling 429 tests. It includes detection tests (181 tests) for app type detection and service selection, Node.js production tests (83 tests) for Express.js and Dockerfile validation, and validation tests (165 tests) for Azure resource naming, Bicep files, and preflight checks. The PR also adds CI workflows for automated testing and repository-wide Copilot instructions.
Changes:
- Added three independent test suites with dedicated package.json files and jest configurations
- Created CI workflows for detection, nodejs-production, and validation test suites
- Added Copilot instructions documenting repository structure, testing guidelines, and skill development practices
Reviewed changes
Copilot reviewed 95 out of 98 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/validation/src/validators/*.js | Validator implementations for resource naming, Bicep files, and preflight checks |
| tests/nodejs-production/src/validators/*.js | Validators for Express.js production best practices and Dockerfile validation |
| tests/detection/src/*.js | Detection logic for app types, file patterns, and service mappings |
| tests/**/package.json | Independent test suite configurations with Jest dependencies |
| .github/workflows/test-*.yml | CI workflows for running test suites on push/PR with codecov integration |
| tests/**/README.md | Documentation for each test suite with setup, running, and coverage details |
| .github/copilot-instructions.md | Repository-wide guidance for skill development and testing |
Comments suppressed due to low confidence (6)
.github/copilot-instructions.md:97
- Lines 95-97 show inconsistent test counts: the file claims "264+ tests" for detection but other documentation states 181 tests. This should be corrected to maintain consistency across documentation.
- **Detection tests**: 264+ tests, all should pass
- **Node.js production tests**: 83+ tests
- **Validation tests**: 165+ tests
.github/copilot-instructions.md:128
- Line 128 references an incorrect path
tests/fixtures/projects/my-new-frameworkbut the actual path should betests/detection/fixtures/projects/my-new-frameworkbased on the test structure shown elsewhere in the file.
mkdir -p tests/fixtures/projects/my-new-framework
.github/copilot-instructions.md:146
- Lines 145-146 reference incorrect paths for detection logic files. The paths should be
tests/detection/src/filePatterns.jsandtests/detection/src/appTypeDetector.jsinstead of thetests/src/detection/prefix shown here.
- `tests/src/detection/filePatterns.js` - Add file patterns
- `tests/src/detection/appTypeDetector.js` - Add detection logic
tests/detection/utils/project-scanner.js:13
- Unused variable SKILL_ROUTES.
tests/detection/utils/project-scanner.js:14 - Unused variable CONFIDENCE_LEVELS.
tests/detection/utils/project-scanner.js:15 - Unused variable createRecommendation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Match: HEALTHCHECK CMD or HEALTHCHECK --interval=30s ... CMD | ||
| // Handles both --option=value and --option value formats | ||
| const healthcheckRegex = /^HEALTHCHECK\s+(?:--[\w-]+=?\S*\s+)*(?:CMD|NONE)/im; |
There was a problem hiding this comment.
The regex pattern on line 141 for healthcheck validation may not correctly handle all valid HEALTHCHECK instruction formats. The current pattern requires options to use the = format (--option=value), but Docker also allows space-separated format (--option value). The pattern --[\w-]+=?\S* will match options, but the ? after = makes it optional yet still requires \S* which could lead to incorrect matching.
Consider using a more robust pattern that explicitly handles both formats: --[\w-]+(?:=\S+|\s+\S+)? or simplifying to just check for the presence of HEALTHCHECK and CMD/NONE without validating the option format in detail.
| └── reference/ # Reference guides (optional) | ||
|
|
||
| tests/ | ||
| ├── detection/ # App type detection tests (264 tests) |
There was a problem hiding this comment.
Line 19 states there are 264 tests in the detection suite, but the PR description and multiple other places in the documentation (line 96, tests/README.md line 96, tests/detection/README.md line 45) state 181 tests. This inconsistency should be corrected to match the actual test count.
|
|
||
| **DO NOT create new test suites or test areas.** Add tests only to existing coverage areas: | ||
|
|
||
| - `tests/__tests__/detection/` - Detection logic tests |
There was a problem hiding this comment.
The copilot-instructions.md file references incorrect paths for test directories. Line 107 mentions tests/__tests__/detection/ but the actual structure is tests/detection/__tests__/. This will cause confusion when developers try to follow these instructions.
This issue also appears in the following locations of the same file:
- line 128
- line 145
| - `tests/__tests__/detection/` - Detection logic tests | |
| - `tests/detection/__tests__/` - Detection logic tests |
| 3. **Update expectations.json** if service mappings change | ||
| 4. **Run all test suites** before committing: | ||
| ```bash | ||
| cd tests && npm test |
There was a problem hiding this comment.
Lines 168-172 show incorrect commands for running test suites. The commands reference just cd tests && npm test but the tests are organized into separate suites (detection, nodejs-production, validation) each with their own package.json. The correct commands should be cd tests/detection && npm test, etc., as shown correctly in lines 82-90.
| cd tests && npm test | |
| cd tests/detection && npm test |
| const path = require('path'); | ||
|
|
||
| const { | ||
| AZURE_SERVICES, |
There was a problem hiding this comment.
Unused variable AZURE_SERVICES.
This issue also appears in the following locations of the same file:
- line 13
- line 14
- line 15
| * Tests for Express.js Production Validator | ||
| */ | ||
|
|
||
| const path = require('path'); |
There was a problem hiding this comment.
Unused variable path.
| suggestShortenedName, | ||
| getResourceConstraints, | ||
| listResourceTypes, | ||
| RESOURCE_CONSTRAINTS |
There was a problem hiding this comment.
Unused variable RESOURCE_CONSTRAINTS.
| * Based on azure-deployment-preflight/SKILL.md detection logic. | ||
| */ | ||
|
|
||
| const fs = require('fs'); |
There was a problem hiding this comment.
Unused variable fs.
| @@ -0,0 +1,389 @@ | |||
| # Azure Skill Tests | |||
|
|
|||
| Automated test suites for Azure deployment skills. These tests verify detection logic, production readiness validation, and Azure resource validation. | |||
There was a problem hiding this comment.
What detection logic is being verified? How is it being verified? How do the tests actually work?
What does "production readiness validation" mean? Is it validating the production readiness of the skills or something else?
|
Had a conversation offline. The tests are for a small number of specific skills: I suggest we break down the PR to focus on testing one skill at a time for easier review. Maybe add one for PoC purpose. We will need to reorganize the test files to leave room for extending tests for other skills. |
|
I think we can close this one - the test infrastructure has evolved significantly since this PR was opened, and we now have a better structure in place with the |
paulyuk
left a comment
There was a problem hiding this comment.
Please take a look. Top suggestion is to get this test bed onto latest versions. This is going to be a lot to maintain and analyze. Hopefully perf ok.
|
|
||
| strategy: | ||
| matrix: | ||
| node-version: [20, 22] |
There was a problem hiding this comment.
at some point later might need to move this to an overall config for actions/workflows so the runners consistently use the rright latest versions.
|
|
||
| ### Key Guidelines | ||
|
|
||
| - **Use `azd` for deployments** - Azure Developer CLI, not `az` CLI |
There was a problem hiding this comment.
We might need stronger guidance to use approved templates documented in skills and MCP tools first. but ok to wait on that.
| 'FunctionApp.csproj': ` | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <TargetFramework>net8.0</TargetFramework> |
There was a problem hiding this comment.
Ok here, but we should start biasing the skills to use supported only versions, like net10.0
| }); | ||
|
|
||
| describe('Generic Java', () => { | ||
| test('detects generic Java Maven project with low confidence', () => { |
There was a problem hiding this comment.
I would expect a stronger Maven / mvn test. BTW That is the more used path for our JAva functions customers.
There was a problem hiding this comment.
Might be worth testing for the bad .funcignore case mentioned here that breaks Oryx server build:
#769
| @@ -0,0 +1,7 @@ | |||
| FROM node:20-alpine | |||
| @@ -0,0 +1,8 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk.Web"> | |||
| <PropertyGroup> | |||
| <TargetFramework>net8.0</TargetFramework> | |||
There was a problem hiding this comment.
| <TargetFramework>net8.0</TargetFramework> | |
| <TargetFramework>net10.0</TargetFramework> |
?
| @@ -0,0 +1,9 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <PropertyGroup> | |||
| <TargetFramework>net8.0</TargetFramework> | |||
There was a problem hiding this comment.
please review all net8.0 instances
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
this is correct for all languages EXCEPT .NET and Powershell.
Summary
Adds comprehensive test coverage for Azure skills with consistent structure, CI workflows, and repository-wide Copilot instructions.
Test Suites (429 total tests)
All suites follow the same independent structure with their own
package.json:tests/detection/tests/nodejs-production/tests/validation/Running Tests
CI Workflows
test-detection.ymlazure-deployskill,tests/detection/test-nodejs-production.ymlazure-nodejs-productionskilltest-validation.ymlazure-validation,azure-deployment-preflightskillsCopilot Instructions
Added
.github/copilot-instructions.mdwith:Key Changes
tests/to consistent suite-per-directory patterntest.yml→test-detection.ymlwith updated pathstest-validation.ymlworkflow