Skip to content

Conversation

@AprilDeFeu
Copy link
Owner

This pull request introduces a comprehensive, multi-language testing and security framework to ensure the quality and integrity of all scripts in the repository.

Key additions include:

  • A .pre-commit-config.yaml with hooks for linting, formatting, and secret scanning.
  • A tests/ directory with Pester unit tests for PowerShell scripts.
  • A GitHub Actions CI workflow to automate linting and testing on pull requests.
  • Updated CONTRIBUTING.md and Copilot instructions to enforce a test-focused development approach.

This commit introduces a comprehensive, multi-language testing and security framework.

- Adds a .pre-commit-config.yaml with hooks for linting (markdown, yaml, powershell), formatting (prettier), and secret scanning (gitleaks).
- Establishes a 	ests/ directory with a unit test for the existing PowerShell script using Pester.
- Implements a GitHub Actions CI workflow (.github/workflows/ci.yml) to run linters and tests automatically on pull requests.
- Adds TESTING.md to document the local testing setup.
- Includes .vscode/ settings for recommended extensions and tasks.
- Updates CONTRIBUTING.md and Copilot instructions to enforce a test-focused development approach, emphasizing edge case testing.
Copilot AI review requested due to automatic review settings November 12, 2025 02:46
@AprilDeFeu
Copy link
Owner Author

@copilot given the failures in the checks, analyze what changes are required to fix this PR and then commit them.

Copy link
Contributor

Copilot AI commented Nov 12, 2025

@AprilDeFeu I've opened a new pull request, #10, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
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 introduces a comprehensive testing and security framework for the Scripts repository, including pre-commit hooks, unit tests, CI automation, and updated documentation to enforce quality standards.

Key Changes

  • Added pre-commit configuration with linting, formatting, and security scanning hooks
  • Implemented Pester unit tests for PowerShell scripts with basic validation and smoke testing
  • Created GitHub Actions CI workflow to automate testing and linting on pull requests

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tests/unit/PowerShell/system-maintenance.Tests.ps1 Pester unit test file for PowerShell system maintenance script with validation and smoke tests
TESTING.md Documentation outlining the testing framework, local testing setup, and CI pipeline
CONTRIBUTING.md Updated contribution guidelines with test-focused development principles and coverage checklist
.vscode/tasks.json VSCode tasks for running linting and PowerShell unit tests
.vscode/extensions.json Recommended VSCode extensions for PowerShell and Prettier
.pre-commit-config.yaml Pre-commit hook configuration for code quality, formatting, and security checks
.github/workflows/ci.yml CI workflow for automated linting and PowerShell testing
.github/instructions/copilot.instructions.md Repository-specific Copilot instructions for testing and quality standards

Comment on lines 36 to 37
# Exclude the test file itself from analysis for now
exclude: '^tests/unit/PowerShell/system-maintenance.Tests.ps1$'
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment references a file path that doesn't match the actual test file location. The exclude pattern is '^tests/unit/PowerShell/system-maintenance.Tests.ps1$' but the comment says "Exclude the test file itself from analysis for now." This is misleading because:

  1. It's unclear why a test file would need to be excluded from PowerShell script analysis
  2. The comment "for now" suggests this is temporary, but there's no explanation of when or why it should be re-enabled
  3. Test files should generally follow the same code quality standards as the scripts they test

Consider either removing this exclusion or providing a clear explanation of why this specific test file needs to be excluded from analysis.

Suggested change
# Exclude the test file itself from analysis for now
exclude: '^tests/unit/PowerShell/system-maintenance.Tests.ps1$'

Copilot uses AI. Check for mistakes.
Copy link
Owner Author

Choose a reason for hiding this comment

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

This file is ignored because it involves a system-wide cleanup, so it is incredibly expensive on the checks.

Comment on lines 34 to 66
# tests/unit/PowerShell/system-maintenance.Tests.ps1

BeforeAll {
# Suppress verbose output from the script itself during tests
$VerbosePreference = 'SilentlyContinue'
# Path to the script being tested
$scriptPath = "$PSScriptRoot/../../../PowerShell/system-administration/maintenance/system-maintenance.ps1"
}

Describe "system-maintenance.ps1" {
Context "Basic Script Validation" {
It "should be a valid script file" {
Test-Path -Path $scriptPath | Should -Be $true
}

It "should have comment-based help" {
$help = Get-Help -Path $scriptPath -ErrorAction SilentlyContinue
$help | Should -Not -BeNull
($help.Name -eq 'system-maintenance') | Should -Be $true
}

It "should support -WhatIf" {
$command = Get-Command -Path $scriptPath
$command.Parameters.Keys | Should -Contain 'WhatIf'
}
}

Context "Execution Smoke Test" {
It "should run without throwing errors with default parameters" {
& $scriptPath -WhatIf | Should -Not -Throw
}
}
}
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The entire test file content is duplicated (lines 1-33 are identical to lines 34-66). This duplication should be removed, keeping only one copy of the test suite.

Suggested change
# tests/unit/PowerShell/system-maintenance.Tests.ps1
BeforeAll {
# Suppress verbose output from the script itself during tests
$VerbosePreference = 'SilentlyContinue'
# Path to the script being tested
$scriptPath = "$PSScriptRoot/../../../PowerShell/system-administration/maintenance/system-maintenance.ps1"
}
Describe "system-maintenance.ps1" {
Context "Basic Script Validation" {
It "should be a valid script file" {
Test-Path -Path $scriptPath | Should -Be $true
}
It "should have comment-based help" {
$help = Get-Help -Path $scriptPath -ErrorAction SilentlyContinue
$help | Should -Not -BeNull
($help.Name -eq 'system-maintenance') | Should -Be $true
}
It "should support -WhatIf" {
$command = Get-Command -Path $scriptPath
$command.Parameters.Keys | Should -Contain 'WhatIf'
}
}
Context "Execution Smoke Test" {
It "should run without throwing errors with default parameters" {
& $scriptPath -WhatIf | Should -Not -Throw
}
}
}

Copilot uses AI. Check for mistakes.

Context "Execution Smoke Test" {
It "should run without throwing errors with default parameters" {
& $scriptPath -WhatIf | Should -Not -Throw
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The test on line 30 uses Should -Not -Throw incorrectly. The Should -Not -Throw assertion requires a script block { } to wrap the command being tested.

Correct syntax:

{ & $scriptPath -WhatIf } | Should -Not -Throw

Copilot uses AI. Check for mistakes.
- id: psscriptanalyzer
name: psscriptanalyzer
entry: pwsh -Command "Invoke-ScriptAnalyzer -Path ."
language: "powershell"
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The language field value should be system instead of "powershell" for local hooks in pre-commit. The language: "powershell" is not a valid pre-commit language type. Use language: system and ensure PowerShell is available in the system PATH.

Suggested change
language: "powershell"
language: system

Copilot uses AI. Check for mistakes.
name: psscriptanalyzer
entry: pwsh -Command "Invoke-ScriptAnalyzer -Path ."
language: "powershell"
types: ["powershell"]
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The types field should use powershell without quotes. Pre-commit type identifiers should not be quoted in arrays.

Correct syntax:

types: [powershell]
Suggested change
types: ["powershell"]
types: [powershell]

Copilot uses AI. Check for mistakes.
Comment on lines +45 to +49
run: |
$result = Invoke-Pester -Path 'tests/unit/PowerShell' -PassThru
if ($result.FailedCount -gt 0) {
exit 1
}
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

Missing the closing brace } for the run block. The PowerShell script block that starts on line 45 is not properly closed, which will cause a workflow syntax error.

Add the closing brace after line 49:

          if ($result.FailedCount -gt 0) {
            exit 1
          }

Copilot uses AI. Check for mistakes.
Copilot AI and others added 2 commits November 12, 2025 02:57
- Changed psscriptanalyzer language from 'powershell' to 'system' in .pre-commit-config.yaml
- Fixed bash syntax error in template.sh (replaced 'else {' with 'else')
- Fixed SC2155 ShellCheck warnings by separating variable declarations and assignments
- Updated markdownlint config to exclude instruction files and be more lenient
- Fixed line length issues in CONTRIBUTING.md
- Added shellcheck disable comment for intentional SCRIPT_DIR in template
- All pre-commit hooks now pass successfully

Co-authored-by: AprilDeFeu <[email protected]>
Fix CI failures: pre-commit configuration and shell script issues
@AprilDeFeu
Copy link
Owner Author

@copilot review this again

Copy link
Contributor

Copilot AI commented Nov 12, 2025

@AprilDeFeu I've opened a new pull request, #11, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
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

Copilot reviewed 11 out of 16 changed files in this pull request and generated no new comments.

Copilot AI and others added 2 commits November 12, 2025 03:39
@AprilDeFeu AprilDeFeu marked this pull request as draft November 12, 2025 04:10
@AprilDeFeu
Copy link
Owner Author

@copilot based off the feedback of the CI Test PowerShell check, create a fix in this draft as another commit.

Copy link
Contributor

Copilot AI commented Nov 12, 2025

@AprilDeFeu I've opened a new pull request, #12, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 3 commits November 12, 2025 04:28
Copilot AI review requested due to automatic review settings November 12, 2025 04:58
Copy link
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

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

Comment on lines +150 to +164
### Test Coverage Checklist
Before submitting, ensure your tests cover the following scenarios:

- [ ] **Happy Path:** Does the script work as expected with valid, typical inputs?
- [ ] **Invalid Inputs:** How does the script handle incorrect, malformed, or unexpected inputs?
(e.g., wrong data types, non-existent files).
- [ ] **Edge Cases:**
- [ ] Empty or null inputs.
- [ ] Very large inputs (e.g., large files, many items).
- [ ] Zero-value inputs (e.g., `0`, `""`).
- [ ] Inputs with special characters or different encodings.
- [ ] **Permissions:** Does the script fail gracefully if it lacks the required permissions for a file,
directory, or network resource?
- [ ] **Dependencies:** How does the script behave if a required dependency (module, command-line tool) is missing?
- [ ] **Concurrency:** If applicable, is the script safe to run multiple times simultaneously? Does it handle file locking?
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The test coverage checklist is comprehensive but not reflected in the actual PowerShell unit tests provided. The sample test file (tests/unit/PowerShell/system-maintenance.Tests.ps1) only includes basic validation and a smoke test with -WhatIf, missing most items from this checklist (invalid inputs, edge cases, permissions, dependencies). Consider adding example tests that demonstrate these scenarios or note that this is an aspirational checklist.

Copilot uses AI. Check for mistakes.
Copy link
Owner Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

@AprilDeFeu AprilDeFeu marked this pull request as ready for review November 12, 2025 05:54
Copilot AI review requested due to automatic review settings November 12, 2025 05:55
Copy link
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

Copilot reviewed 12 out of 16 changed files in this pull request and generated 2 comments.

Copy link
Contributor

Copilot AI commented Nov 12, 2025

@AprilDeFeu I've opened a new pull request, #13, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 2 commits November 12, 2025 06:12
Add comprehensive test examples demonstrating test coverage checklist
@AprilDeFeu
Copy link
Owner Author

@copilot fully review this PR one more time

Copy link
Contributor

Copilot AI commented Nov 12, 2025

@AprilDeFeu I've opened a new pull request, #14, to work on those changes. Once the pull request is ready, I'll request review from you.

Complete comprehensive PR review with validation of testing and security framework
@AprilDeFeu AprilDeFeu merged commit 67d31fe into main Nov 12, 2025
5 checks passed
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.

2 participants