Skip to content

test: add powershell module detection and execution tests#8373

Merged
rubenfiszel merged 1 commit intomainfrom
test-powershell-module-loading
Mar 15, 2026
Merged

test: add powershell module detection and execution tests#8373
rubenfiszel merged 1 commit intomainfrom
test-powershell-module-loading

Conversation

@rubenfiszel
Copy link
Contributor

Summary

Adds test coverage for the PowerShell module loading fixes from #8370 and #8371.

18 unit tests in pwsh_executor.rs covering:

  • RE_POWERSHELL_IMPORTS regex: basic, indented (spaces/tabs), quoted names, -Name flag, -RequiredVersion, commented-out imports
  • get_module_versions(): empty dir, empty version subdir, valid versioned module, flat module with/without files, multiple versions
  • check_module_installed(): empty dir returns false, valid module returns true, wrong version returns false

2 new e2e integration tests in tests/worker.rs:

  • test_powershell_param_block_with_attributes: param block with [Parameter] attributes and typed params
  • test_powershell_error_caught: verifies catch block outputs "An error occurred:" and the error message without leaking literal "Write-Output" text

Test plan

  • cargo test -p windmill-worker pwsh_executor::tests — 18/18 passed
  • cargo test --test worker test_powershell — 3/3 passed (including existing test_powershell_job)

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude
Copy link
Contributor

claude bot commented Mar 14, 2026

Claude finished @rubenfiszel's task in 2m 40s —— View job


Review of PR #8373: test: add powershell module detection and execution tests

  • Gather context and read changed files
  • Review unit tests in pwsh_executor.rs
  • Review integration tests in worker.rs
  • Review Cargo.toml changes
  • Post review feedback

Overall this is a solid test PR that adds good coverage for the PowerShell module loading fixes in #8370 and #8371. The tests are well-structured, clearly named, and cover the important edge cases. A few observations:

Feedback

1. Regression assertion in test_powershell_error_caught could be fragile (backend/tests/worker.rs:1598-1601)

The assertion checking that Write-Output doesn't appear after the error marker uses starts_with("\\nWrite-Output") — note the escaped \\n, which looks for the literal two characters \n in the JSON-serialized string, not a newline. This works because serde_json::to_string serializes newlines as \n literals, but it's subtle. A comment explaining this would help future readers, or consider using contains("Write-Output") on the after_marker substring for clarity:

assert!(
    !after_marker.contains("Write-Output"),
    "catch block should not output literal 'Write-Output' text, got: {result_str}"
);

This is a minor nit — the current code is correct, just slightly less readable than it could be.

2. Missing test for module not in module_dirs HashMap

check_module_installed has an else branch that returns (false, Vec::new()) when the module name isn't found in the HashMap at all (line 253-255). There's no test for this case — the existing tests always insert the module key. Consider adding a one-liner:

#[tokio::test]
async fn test_check_module_not_in_dirs() {
    let dirs = HashMap::new();
    let (installed, versions) = check_module_installed(&dirs, "NonExistent", None).await.unwrap();
    assert!(!installed);
    assert!(versions.is_empty());
}

3. Unit tests are clean and well-organized — The regex tests cover the important patterns (basic, indented, quoted, -Name flag, version, comments). The filesystem tests using TempDir are properly isolated and clean up after themselves.

4. Cargo.toml change looks correcttempfile is already a workspace dependency, so adding it as a dev-dependency for windmill-worker is the right approach.

5. Integration tests add good e2e coverage — Testing param blocks with [Parameter] attributes and the error catch-block behavior are practical scenarios that validate the PowerShell executor end-to-end.

How to verify through the app

These are backend unit and integration tests, so they cannot be verified through the app UI. To verify: check out the branch and run cargo test -p windmill-worker pwsh_executor::tests for the 18 unit tests, and cargo test --test worker test_powershell for the 3 e2e integration tests (requires a running PostgreSQL instance and PowerShell installed).

@cloudflare-workers-and-pages
Copy link

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: b46dbeb
Status: ✅  Deploy successful!
Preview URL: https://4b659009.windmill.pages.dev
Branch Preview URL: https://test-powershell-module-loadi.windmill.pages.dev

View logs

@rubenfiszel rubenfiszel merged commit 5296ade into main Mar 15, 2026
14 checks passed
@rubenfiszel rubenfiszel deleted the test-powershell-module-loading branch March 15, 2026 04:11
@github-actions github-actions bot locked and limited conversation to collaborators Mar 15, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant