Repository: microsoft/terminal (language: cpp)

Issue:
GetProfileForArgsWithCommandline unit test is failing
### Windows Terminal version

Commit 10c963a7dbdd0d9e71dd0596e96fa26ab88a76f5

### Windows build number

10.0.19041.1415

### Other Software

_No response_

### Steps to reproduce

1. Checkout the latest code.
2. Build.
3. Run the `GetProfileForArgsWithCommandline` unit test.


### Expected Behavior

Test should pass.

### Actual Behavior

Test fails with the following log:

```
StartGroup: SettingsModelLocalTests::TerminalSettingsTests::GetProfileForArgsWithCommandline
Verify: IsNotNull(profile)
Verify: AreEqual(expectedGUID, static_cast<const GUID&>(profile.Guid()))
Verify: IsNotNull(profile)
Error: Verify: AreEqual(expectedGUID, static_cast<const GUID&>(profile.Guid())) - Values ({6239A42C-1111-49A3-80BD-E8FDD045185C}, {6239A42C-0000-49A3-80BD-E8FDD045185C}) [File: C:\Users\James\CPP\terminal\src\cascadia\LocalTests_SettingsModel\TerminalSettingsTests.cpp, Function: SettingsModelLocalTests::TerminalSettingsTests::GetProfileForArgsWithCommandline, Line: 249]
EndGroup: SettingsModelLocalTests::TerminalSettingsTests::GetProfileForArgsWithCommandline [Failed]
```

I think this is a fairly recent break. I haven't done a bisect, but I'd guess it might have come from PR #12348, just based on the code involved.


Discussion / hints:
@lhecker as an fyi
I forgot that we don't run these tests in our CI. Uh oh. 😟
I made a off-by-one error in #12348. 😞
The impact is: All command-lines with exactly one additional argument (aka: `argc == 2`) are normalized to not having any arguments (`argc == 1`). The good news is that this happens to both profiles and handed off processes symmetrically so the impact is somewhat limited at least (unlike before #12348, where paths with spaces just didn't work at all).

I've got a fix ready, but I'm sad we _just_ now re-released 1.12/1.13. I should look into how we can run those tests as part of our CI in the future, so that this can't happen anymore, even if someone neglects to run those tests - someone like me. 😞
> I should look into how we can run those tests as part of our CI in the future

We started looking into it: #10267

I'm not sure if the pool has updated the machines. IIRC the localtests needed a newer version of the OS than what the CI runs, but that was also two years ago now.
Gah, I tried in https://dev.azure.com/ms/terminal/_build/results?buildId=272194&view=logs&j=a9c0e965-911d-5d8d-6a81-36c112a94df3&t=9660b670-796f-5b84-9008-12dc9f750614.

```
StartGroup: SettingsModelLocalTests::TerminalSettingsTests::TestCommandlineToTitlePromotion
EndGroup: SettingsModelLocalTests::TerminalSettingsTests::TestCommandlineToTitlePromotion [Blocked]
TAEF: Deploying custom AppXManifest... Copying C:\a\_work\1\a\drop\Release\x64\test\TestHostAppXManifest.xml to C:\a\_work\1\a\drop\Release\x64\test\AppXManifest.xml
TAEF: Restoring cached AppXManifest... Copying C:\Users\CLOUDT~1\AppData\Local\Temp\Tfm-4160-0\AppXManifest.xml to C:\a\_work\1\a\drop\Release\x64\test\AppXManifest.xml
Error: TAEF: [HRESULT: 0x8000401A] Failed to create the test host process for out of process test execution. (Failed to enable debugging for 'WindowsTerminal.TestHost_1.0.0.0_x64__aq7gd5554gxm8'.)

```

Probably the same thing as always. I idly wonder if we can just ship the vclibs sxs in the test like we do for the actual package. #3838 was the thread I was using before, but Helix is probably an easier solution.

Fix patch:
diff --git a/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
index 841c43a8827..4f1ce422c14 100644
--- a/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
+++ b/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
@@ -199,6 +199,10 @@ namespace SettingsModelLocalTests
                         "guid": "{6239a42c-3333-49a3-80bd-e8fdd045185c}",
                         "commandline": "cmd.exe /A /C",
                         "connectionType": "{9a9977a7-1fe0-49c0-b6c0-13a0cd1c98a1}"
+                    },
+                    {
+                        "guid": "{6239a42c-4444-49a3-80bd-e8fdd045185c}",
+                        "commandline": "C:\\invalid.exe",
                     }
                 ]
             }
@@ -217,7 +221,7 @@ namespace SettingsModelLocalTests
             TestCase{ L"cmd.exe", 0 },
             // SearchPathW() normalization + case insensitive matching.
             TestCase{ L"cmd.exe /a", 1 },
-            TestCase{ L"C:\\Windows\\System32\\cmd.exe /A", 1 },
+            TestCase{ L"%SystemRoot%\\System32\\cmd.exe /A", 1 },
             // Test that we don't pick the equally long but different "/A /B" variant.
             TestCase{ L"C:\\Windows\\System32\\cmd.exe /A /C", 1 },
             // Test that we don't pick the shorter "/A" variant,
@@ -227,6 +231,9 @@ namespace SettingsModelLocalTests
             // Ignore profiles with a connection type, like the Azure cloud shell.
             // Instead it should pick any other prefix.
             TestCase{ L"C:\\Windows\\System32\\cmd.exe /A /C", 1 },
+            // Failure to normalize a path (e.g. because the path doesn't exist)
+            // should yield the unmodified input string (see NormalizeCommandLine).
+            TestCase{ L"C:\\invalid.exe /A /B", 4 },
             // Return base layer profile for missing profiles.
             TestCase{ L"C:\\Windows\\regedit.exe", -1 },
         };
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
index fba878395e9..4935cee4b9c 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
@@ -695,11 +695,6 @@ std::wstring CascadiaSettings::NormalizeCommandLine(LPCWSTR commandLine)
     // The index of the first argument in argv for our executable in argv[0].
     // Given {"C:\Program Files\PowerShell\7\pwsh.exe", "-WorkingDirectory", "~"} this will be 1.
     int startOfArguments = 1;
-    // Returns true if the executable in argv[0] to argv[startOfArguments - 1]
-    // has any further arguments in argv[startOfArguments] to argv[argc - 1].
-    const auto hasTrailingArguments = [&]() noexcept {
-        return (argc - startOfArguments) > 1;
-    };
 
     // The given commandLine should start with an executable name or path.
     // For instance given the following argv arrays:
@@ -754,7 +749,7 @@ std::wstring CascadiaSettings::NormalizeCommandLine(LPCWSTR commandLine)
         // Just like CreateProcessW() we thus try to concatenate arguments until we successfully resolve a valid path.
         // Of course we can only do that if we have at least 2 remaining arguments in argv.
         // All other error types aren't handled at the moment.
-        else if (!hasTrailingArguments() || status != HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
+        else if ((argc - startOfArguments) < 2 || status != HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
         {
             break;
         }
@@ -770,7 +765,7 @@ std::wstring CascadiaSettings::NormalizeCommandLine(LPCWSTR commandLine)
     // We're now going to append all remaining arguments to the resulting string.
     // If argv is {"C:\Program Files\PowerShell\7\pwsh.exe", "-WorkingDirectory", "~"},
     // then we'll get "C:\Program Files\PowerShell\7\pwsh.exe\0-WorkingDirectory\0~"
-    if (hasTrailingArguments())
+    if (startOfArguments < argc)
     {
         // normalized contains a canonical form of argv[0] at this point.
         // -1 allows us to include the \0 between argv[0] and argv[1] in the call to append().


Test patch: