Add tests for template file syntax errors #3962
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces comprehensive test coverage for shell script template files to prevent syntax errors from being introduced into production template files. The tests validate that template files have valid syntax by processing them with shell interpreters and checking for errors.
- Adds a new test class
ShellScriptSyntaxL0with syntax validation tests for shell script templates - Includes both positive tests (valid syntax) and negative tests (intentionally broken syntax) to ensure the validation mechanism works correctly
- Covers multiple template files including update scripts and service scripts for different platforms
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/Test/L0/Listener/ShellScriptSyntaxL0.cs | New test class implementing comprehensive syntax validation for shell script and cmd templates |
| src/Misc/layoutbin/update.sh.template | Minor formatting change splitting if-then statement across multiple lines |
| string templatePath = Path.Combine(tempDir, "complex_shell.sh.template"); | ||
|
|
||
| // Write a sample template with various shell features | ||
| string template = @"#!/bin/bash |
There was a problem hiding this comment.
What are we testing with these? 🤔
There was a problem hiding this comment.
This test is valuable to keep because it tests complex shell syntax patterns that might not be present in our actual template files. It serves as validation for our syntax checking framework itself, not just our current templates.
The test covers important shell script features like:
- Nested quotes and complex quoting patterns
- Function definitions with parameter handling
- Command substitution and string manipulation
- Here documents
These patterns might not all exist in our current templates, but having this test ensures our validation system can handle them if we ever add them. It's basically testing the validator itself rather than just the templates.
I'd vote to keep it as a solid regression test for our validation framework, in-case we modify it going forward.
There was a problem hiding this comment.
Should we just replace all the placeholder strings in the script and then run bash -n against it?
So we don't need to add any 3rd party dependency.
There was a problem hiding this comment.
With just bash -n it wasn't able to catch all the syntax errors, I tried making various syntax errors and they would not get detected, missing brackets for example would not get detected at all
There was a problem hiding this comment.
For example, tests pass with missing bracket with bash -n and no shellcheck
There was a problem hiding this comment.
With shell check it gives an error
There was a problem hiding this comment.
I have it so that if the dependency isn't there, it just uses bash -n still though
There was a problem hiding this comment.
Now i am wondering whether we should just run the script for testing.
We want the test to be reliable, given the facts that bash -n might not catch expression error, and shellcheck may not exist on the CI machine, actually execute the script might be our best way to always test the script's syntax errors.
2 participants
Introduce tests to ensure that template files do not contain syntax errors. This enhances code reliability and maintainability.
Using shellcheck for syntax checking for sh template files https://github.com/koalaman/shellcheck
Aims to avoid issues like this #3955