WithExpectedExitCode ignored when the tool exit code is zero

[Jericho]

Prerequisites

• [X] I have written a descriptive issue title
• [X] I have searched issues to ensure it has not already been reported

Cake runner

Cake .NET Tool

Cake version

2.3.0

Operating system

Windows

Operating system architecture

64-Bit

CI Server

No response

What are you seeing?

This bug is related to the new feature added in Cake 2.3.0.

Firstly, let's say I have an executable built with C# with the following signature static async Task Main(string[] args) (notice Task rather than Task<int>). In other words, my executable does not return a numerical exit code and therefore the exit code is assumed to be zero. I haven't tested it, but I am assuming you could also reproduce this exact same problem with an executable with the following signature: static async Task<int> Main(string[] args) coupled with this line in the body of the "Main": return 0;.

What's important in order to reproduce the problem I am describing is that the exit code of the executable must be zero. I fully understand that zero typically signifies that the tool completed successfully but that's not always the case.

Secondly, let's say that my Cake script contains the following:

Command(
    new CommandSettings
    {
        ToolName = appName,
        ToolExecutableNames = new[] { appName, $"{appName}.exe" },
        ToolPath = $"{publishDir}{appName}.exe",
    }.WithExpectedExitCode(1234),
    new ProcessArgumentBuilder()
        .Append("nopause")
};

Notice that I specified "1234" as the expected exit code. In this scenario, my executable returns zero and I have specified that the expected exit code should be 1234. I expect an exception to be thrown to let me know that the exit code does not match the expected value but that's not what I observe. I observe that my Cake script completes successfully, and no exception is thrown.

I investigated this situation and I believe I discovered what's causing this problem in Cake.Core/Tooling/Tool.cs. Line 101 invokes the custom exit code handler and correctly identifies that zero is not the expected exit code and line 103 invokes ProcessExitCode. However, the logic in the ProcessExitCode method on line 115 throws an exception IF AND ONLY IF the exit code is different than zero which, in the scenario I presented is not the desired behavior.

https://github.com/cake-build/cake/blob/2e23e6f647e00e8ab0e8dc5a9ec2382f99659b45/src/Cake.Core/Tooling/Tool.cs#L101..L120

Again, let me repeat that I completely understand that a "zero" exit code typically signifies success, but I don't think Cake should hard code this logic which has the side effect of overriding the custom exit code handler as I have demonstrated in my scenario.

What is expected?

I expect an exception to be thrown which would let me know that the exit code does not match the expected value.

Steps to Reproduce

As I stated previously, there are two conditions that must be met to reproduce this problem:

1. The executable must return zero (either because Task<int> Main returns zero or Task Main does not return a numerical exit code which consequently is assumed to be zero)
2. The CommandSettings when invoking Command must define a non-zero expected exit code

When these two conditions are met, I expect an exception but instead my Cake script completes successfully.

Output log

No response

[devlead]

Hmm not sure I'm following if I have this console

public static class Program
{
    public static async Task Main(string[] args) 
    {
        await Task.CompletedTask;
    }
}

Executing that will return 0 in PowerShell

> dotnet run
> $LASTEXITCODE
0

And in Cake

Context.Tools.RegisterFile(MakeAbsolute(File("./bin/Debug/net7.0/Test.exe")));

Command(
    new [] { "Test.exe", "Test"}
);

when executed

If I change co

public static class Program
{
    public static async Task Main(string[] args) 
    {
        await Task.CompletedTask;
        throw new Exception("WOW");
    }
}

it won't return zero in PowerShell

> dotnet run
> $LASTEXITCODE
Unhandled exception. System.Exception: WOW
   at Program.Main(String[] args) in C:\temp\priv\test\Test\Program.cs:line 6
   at Program.<Main>(String[] args)
                                   -532462766

And in Cake

Context.Tools.RegisterFile(MakeAbsolute(File("./bin/Debug/net7.0/Test.exe")));

Command(
    new [] { "Test.exe", "Test"}
);

when executed

Unhandled exception. System.Exception: WOW
   at Program.Main(String[] args) in C:\temp\priv\test\Test\Program.cs:line 6
   at Program.<Main>(String[] args)
Error: Test.exe: Process returned an error (exit code -532462766).

And with expectedExitCode -532462766

Context.Tools.RegisterFile(MakeAbsolute(File("./bin/Debug/net7.0/Test.exe")));

Command(
    new [] { "Test.exe", "Test"},
    expectedExitCode: -532462766
);

it won't fail

Unhandled exception. System.Exception: WOW
   at Program.Main(String[] args) in C:\temp\priv\test\Test\Program.cs:line 6
   at Program.<Main>(String[] args)

same if I remove the throw in console

public static class Program
{
    public static async Task Main(string[] args) 
    {
        await Task.CompletedTask;
    }
}

it won't fail, am I right to say that that's what unexpected? And you wanted an error like

Error: Test.exe: Process returned an error (exit code 0).

?

That would mean something like

diff --git a/src/Cake.Common/Tools/Command/CommandAliases.cs b/src/Cake.Common/Tools/Command/CommandAliases.cs
index 7f1f3fb0..e32aa486 100644
--- a/src/Cake.Common/Tools/Command/CommandAliases.cs
+++ b/src/Cake.Common/Tools/Command/CommandAliases.cs
@@ -8,6 +8,7 @@ using System.Linq;
 using Cake.Core;
 using Cake.Core.Annotations;
 using Cake.Core.IO;
+using Cake.Core.Tooling;

 namespace Cake.Common.Tools.Command
 {
@@ -425,8 +426,7 @@ namespace Cake.Common.Tools.Command
             {
                 ToolName = toolExecutableNames.First(),
                 ToolExecutableNames = toolExecutableNames,
-                HandleExitCode = exitCode => exitCode == expectedExitCode
-            };
+            }.WithExpectedExitCode(expectedExitCode);

             return settingsCustomization?.Invoke(settings) ?? settings;
         }
diff --git a/src/Cake.Core/Tooling/ToolSettingsExtensions.cs b/src/Cake.Core/Tooling/ToolSettingsExtensions.cs
index 4011095f..72e1cef4 100644
--- a/src/Cake.Core/Tooling/ToolSettingsExtensions.cs
+++ b/src/Cake.Core/Tooling/ToolSettingsExtensions.cs
@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.

 using System;
+using System.Globalization;
 using Cake.Core.IO;

 namespace Cake.Core.Tooling
@@ -147,6 +148,8 @@ namespace Cake.Core.Tooling
         /// <returns>The tools settings.</returns>
         public static T WithExpectedExitCode<T>(this T toolSettings, int expectExitCode)
                where T : ToolSettings
-            => toolSettings.WithToolSettings(toolSettings => toolSettings.HandleExitCode = exitCode => exitCode == expectExitCode);
+            => toolSettings.WithHandleExitCode(exitCode => exitCode == expectExitCode
+                                                ? true
+                                                : throw new CakeException(exitCode, string.Format(CultureInfo.InvariantCulture, "Process returned an unexpected exit code {0}.", exitCode)));
     }
 }

But regardless a process will always have an exit code. A work around, for now, would be to use WithHandleExitCode and supply your own func.

[Jericho]

@devlead to reproduce the problem I outline you must meet two conditions:

1. The executable must return zero
2. You must specify a non-zero expected exit code in your Cake script

In this scenario I expect an exception to be thrown by Cake because the exit code does not match the expected value. However, I observe that my Cake script completes successfully, and no exception is thrown.

As I have stated, I believe the root of this issue is that zero is hard coded in Cake.Core/Tooling/Tool.cs on line 115.

[devlead]

The issue is that exit code is marked as handled if exitCode == expectExitCode, but otherwise it's handled by standard tool execution, so essentially zero or handled.

This is why to change this behavior something like above

-            => toolSettings.WithToolSettings(toolSettings => toolSettings.HandleExitCode = exitCode => exitCode == expectExitCode);
+            => toolSettings.WithHandleExitCode(exitCode => exitCode == expectExitCode
+                                                ? true
+                                                : throw new CakeException(exitCode, string.Format(CultureInfo.InvariantCulture, "Process returned an unexpected exit code {0}.", exitCode)));
     }

to work like you describe above.

Unsure if this is the desired behavior, but above diff would result in that behavior change.

[Jericho]

@devlead Here's unit test to demonstrate the problem

[Fact]
// This unit test demonstrates the problem reported in GH-4013.
// The command's exit code is zero and the expected exit code is any non-zero value (I used '1234' for demonstration purposes).
// In this scenario, I expect Cake to throw an exception because the exit code does not match the expected value.
// Currently this unit test fails because Cake does not throw the expected exception.
// The reason why Cake currently does not throw an exception is because zero is hard-coded on line 115 in Cake.Core/Tooling.Tol.cs
// Zero is hard coded to mean that the command was successful, and the expected exit code is completely disregarded.
public void Exception_Thrown_when_command_exit_code_is_zero_and_expected_value_is_non_zero()
{
    // Given
    const int actualExitCode = 0;
    const int expectedExitCode = 1234;

    var fixture = new CommandRunnerFixture();

    fixture.Settings.WithExpectedExitCode(expectedExitCode);
    fixture.ProcessRunner.Process.SetExitCode(actualExitCode);

    // When
    var result = Record.Exception(() => fixture.Run());

    // Then
    AssertEx.IsCakeException(result, "dotnet: Process returned an error (exit code 0).");
}

[Jericho]

The issue is that exit code is marked as handled if exitCode == expectExitCode

This is exactly the problem. In the scenario I'm presenting, the exit code is zero and the expected exit code is non-zero. They are not equal and therefore I expect Cake to throw an exception.

[devlead]

Yes, but the delegate is used for if exit code is essentially already handled or not, and that's what it does. So if a behavior should be changed it IMHO wouldn't be there but in the WithExpectedExitCode extension method.

[Jericho]

Look at the ProcessExitCode method in Cake.Core/Tooling/Tool.cs. The logic to throw an exception when exit code does not match expected value is there. The issue is that zero is hard coded to be an indication of success no matter what the expected exit code is.

[Jericho]

If the executable returns a value like '999' for example and the expected exit code is '1234', Cake will throw an exception because they don't match.

Why would Cake not throw an exception when the exit code is zero and the expected exit code is '1234'? Why would Cake behave differently in this scenario?

[devlead]

Yes but it's only processed if HandleExitCode returns false. An expected exit code is something that was added with Command without changing the internals of the Cake process, so Zero is the only known good it has. To only allow it to return true or failed would mean changing the internals or changing the delegate as in the example above in the extension method.

[Jericho]

Yes but it's only processed if HandleExitCode returns false.

Correct. And currently HandleExitCode does indeed return false because zero is not equal to '1234'. But this is completely overridden in the ProcessExitCode method because of the if (exitCode != 0) logic. This hard coded logic completely overrules the fact that HandleExitCode has returned false.

In my opinion, an exception should ALWAYS be thrown when HandleExitCode returns false. Currently, that's not the case. I am highlighting the fact that there is one scenario where HandleExitCode returns false, but Cake does not throw the expected exception.

[Jericho]

My suggestion is to replace this code:

if (!settings.HandleExitCode?.Invoke(exitCode) ?? true)
{
    ProcessExitCode(process.GetExitCode());
}

with something like this:

if (!settings.HandleExitCode?.Invoke(exitCode) ?? true)
{
    const string message = "{0}: Process returned an error (exit code {1}).";
    throw new CakeException(exitCode, string.Format(CultureInfo.InvariantCulture, message, GetToolName(), exitCode));
}
else
{
    ProcessExitCode(process.GetExitCode());
}

The goal is to always throw an exception when HandleExitCode returns false (this would fix the problem I highlighted) and invoke ProcessExitCode when it returns true to preserve the current behavior.