Skip to content

Add tests for template file syntax errors #3962

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add tests for template file syntax errors #3962

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3aef228
Add tests for making sure that template files have no syntax errors
salmanmkc Jul 27, 2025
f06a452
update with temp file
salmanmkc Jul 27, 2025
c824407
better validations
salmanmkc Jul 27, 2025
63ada10
update
salmanmkc Jul 27, 2025
ffc2086
skip win 64
salmanmkc Jul 27, 2025
2dc3c3a
Enhance shell script validation with detailed debugging and ShellChec…
salmanmkc Jul 28, 2025
577c73e
don't skip on windows
salmanmkc Jul 28, 2025
49b30c8
add skip back for windows
salmanmkc Jul 28, 2025
1f9418d
update
salmanmkc Jul 28, 2025
820bcaf
Update generic methdod to shelcheck all files that can be
salmanmkc Jul 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
skip win 64
  • Loading branch information
@salmanmkc
salmanmkc committed Jul 27, 2025
commit ffc2086972ce759e2317324e2c9f25e99df3c218
36 changes: 25 additions & 11 deletions src/Test/L0/Listener/ShellScriptSyntaxL0.cs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Fr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, tests pass with missing bracket with bash -n and no shellcheck
test pass with missing bracket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With shell check it gives an error
syntax error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have it so that if the dependency isn't there, it just uses bash -n still though

Copy link
Member

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,12 @@ public void RunHelperShTemplateWithErrorsFailsValidation()
[Trait("SkipOn", "windows")]
public void ValidateShellScript_MissingTemplate_ThrowsException()
{
// Skip on Windows platforms - more explicit check to ensure it's skipped on all Windows variants
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
return;
}

// Test for non-existent template file
// The ValidateShellScriptTemplateSyntax method has a try-catch that will catch and wrap FileNotFoundException
// so we need to test that it produces the appropriate error message
Expand All @@ -296,6 +302,12 @@ public void ValidateShellScript_MissingTemplate_ThrowsException()
[Trait("SkipOn", "windows")]
public void ValidateShellScript_ComplexScript_ValidatesCorrectly()
{
// Skip on Windows platforms - more explicit check to ensure it's skipped on all Windows variants
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
return;
}

// Create a test template with complex shell scripting patterns
string tempDir = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString());
Directory.CreateDirectory(tempDir);
Expand Down Expand Up @@ -396,27 +408,29 @@ public void UpdateCmdTemplateWithErrorsFailsValidation()
[Trait("Level", "L0")]
[Trait("Category", "Runner")]
[Trait("SkipOn", "osx,linux")]
public void ValidateCmdScript_MissingTemplate_ThrowsException()
public void ValidateCmdScript_MissingTemplate_ThrowsFileNotFoundException()
{
// Skip on non-Windows platforms
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
return;
}

// Test for non-existent template file
// The ValidateCmdScriptTemplateSyntax method has a try-catch that will catch and wrap FileNotFoundException
// so we need to test that it produces the appropriate error message
try
// For Windows, we need to use a direct try-catch because File.ReadAllText will throw
// FileNotFoundException if file doesn't exist
try
{
ValidateCmdScriptTemplateSyntax("non_existent_template.cmd.template", shouldPass: true);
Assert.Fail("Expected exception was not thrown");
// This should throw a FileNotFoundException right away
string rootDirectory = Path.GetFullPath(Path.Combine(TestUtil.GetSrcPath(), ".."));
string templatePath = Path.Combine(rootDirectory, "src", "Misc", "layoutbin", "non_existent_template.cmd.template");
string content = File.ReadAllText(templatePath);

// If we get here, the file somehow exists, which should not happen
Assert.Fail($"Expected FileNotFoundException was not thrown for {templatePath}");
}
catch (Exception ex)
catch (FileNotFoundException)
{
// Verify the exception message contains information about the missing file
Assert.Contains("non_existent_template.cmd.template", ex.Message);
Assert.Contains("FileNotFoundException", ex.Message);
// This is expected, so test passes
}
}

Expand Down
Loading