Skip to content

Console.Unix: fix inverted TreatControlCAsInput #42432

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.

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

Merged
merged 1 commit into from
Sep 19, 2020

Conversation

tmds
Copy link
Member

@tmds tmds commented Sep 18, 2020

The logical negation was unintentionally removed while refactoring.

Fixes #42423.
Regressed in #39206.

cc @stephentoub @eiriktsarpalis

The logical negation was unintentionally removed while refactoring.
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 18, 2020
@tmds
Copy link
Member Author

tmds commented Sep 18, 2020

I'm going to add a manual test.

@tmds
Copy link
Member Author

tmds commented Sep 18, 2020

I wrote the test but it doesn't work as I hoped because a parent process already provides 'Ctrl+C' logic to cancel the on-going dotnet test.

$ dotnet test --filter "FullyQualifiedName=System.ConsoleManualTests.TreatControlCAsInput"
  Determining projects to restore...
  All projects are up-to-date for restore.
  System.Security.Principal.Windows -> /home/tmds/repos/runtime/artifacts/bin/System.Security.Principal.Windows/ref/netstandard2.0-Debug/System.Security.Principal.Windows.dll
  System.Security.AccessControl -> /home/tmds/repos/runtime/artifacts/bin/System.Security.AccessControl/ref/netstandard2.0-Debug/System.Security.AccessControl.dll
  Microsoft.Win32.Registry -> /home/tmds/repos/runtime/artifacts/bin/Microsoft.Win32.Registry/ref/netstandard2.0-Debug/Microsoft.Win32.Registry.dll
  System.Runtime -> /home/tmds/repos/runtime/artifacts/bin/System.Runtime/ref/net5.0-Debug/System.Runtime.dll
  System.Security.Principal -> /home/tmds/repos/runtime/artifacts/bin/System.Security.Principal/ref/net5.0-Debug/System.Security.Principal.dll
  Microsoft.Win32.Primitives -> /home/tmds/repos/runtime/artifacts/bin/Microsoft.Win32.Primitives/ref/net5.0-Debug/Microsoft.Win32.Primitives.dll
  System.Collections -> /home/tmds/repos/runtime/artifacts/bin/System.Collections/ref/net5.0-Debug/System.Collections.dll
  System.Runtime.Extensions -> /home/tmds/repos/runtime/artifacts/bin/System.Runtime.Extensions/ref/net5.0-Debug/System.Runtime.Extensions.dll
  System.ComponentModel -> /home/tmds/repos/runtime/artifacts/bin/System.ComponentModel/ref/net5.0-Debug/System.ComponentModel.dll
  System.Runtime.InteropServices -> /home/tmds/repos/runtime/artifacts/bin/System.Runtime.InteropServices/ref/net5.0-Debug/System.Runtime.InteropServices.dll
  System.ObjectModel -> /home/tmds/repos/runtime/artifacts/bin/System.ObjectModel/ref/net5.0-Debug/System.ObjectModel.dll
  System.Collections.NonGeneric -> /home/tmds/repos/runtime/artifacts/bin/System.Collections.NonGeneric/ref/net5.0-Debug/System.Collections.NonGeneric.dll
  System.Memory -> /home/tmds/repos/runtime/artifacts/bin/System.Memory/ref/net5.0-Debug/System.Memory.dll
  System.ComponentModel.Primitives -> /home/tmds/repos/runtime/artifacts/bin/System.ComponentModel.Primitives/ref/net5.0-Debug/System.ComponentModel.Primitives.dll
  System.Net.Primitives -> /home/tmds/repos/runtime/artifacts/bin/System.Net.Primitives/ref/net5.0-Debug/System.Net.Primitives.dll
  System.Collections.Specialized -> /home/tmds/repos/runtime/artifacts/bin/System.Collections.Specialized/ref/net5.0-Debug/System.Collections.Specialized.dll
  System.Net.WebSockets -> /home/tmds/repos/runtime/artifacts/bin/System.Net.WebSockets/ref/net5.0-Debug/System.Net.WebSockets.dll
  System.Security.Claims -> /home/tmds/repos/runtime/artifacts/bin/System.Security.Claims/ref/net5.0-Debug/System.Security.Claims.dll
  System.Security.Principal.Windows -> /home/tmds/repos/runtime/artifacts/bin/System.Security.Principal.Windows/ref/net5.0-Debug/System.Security.Principal.Windows.dll
  System.Security.AccessControl -> /home/tmds/repos/runtime/artifacts/bin/System.Security.AccessControl/ref/net5.0-Debug/System.Security.AccessControl.dll
  TestUtilities -> /home/tmds/repos/runtime/artifacts/bin/TestUtilities/net5.0-Debug/TestUtilities.dll
  System.Console.Manual.Tests -> /home/tmds/repos/runtime/artifacts/bin/System.Console.Manual.Tests/net5.0-Debug/System.Console.Manual.Tests.dll
Test run for /home/tmds/repos/runtime/artifacts/bin/System.Console.Manual.Tests/net5.0-Debug/System.Console.Manual.Tests.dll (.NETCoreApp,Version=v5.0)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
This is a test of TreatControlCAsInput=False. Please type 'Ctrl+C':
Attempting to cancel the build...
Attempting to cancel the build...
Attempting to cancel the build...
Attempting to cancel the build...
Attempting to cancel the build...
Attempting to cancel the build...
Attempting to cancel the build...
Attempting to cancel the build...
Attempting to cancel the build...
Attempting to cancel the build...
Attempting to cancel the build...
Attempting to cancel the build...
Attempting to cancel the build...
Attempting to cancel the build...
/home/tmds/repos/runtime/.dotnet/sdk/5.0.100-rc.1.20454.5/Microsoft.TestPlatform.targets(32,5): error MSB4181: The "Microsoft.TestPlatform.Build.Tasks.VSTestTask" task returned false but did not log an error. [/home/tmds/repos/runtime/src/libraries/System.Console/tests/ManualTests/System.Console.Manual.Tests.csproj]
Attempting to cancel the build...
Did you see no characters were echoed.? [y/n] Attempting to cancel the build...

This is the test:

        [ConditionalTheory(nameof(ManualTestsEnabled))]
        [InlineData(false)]
        [InlineData(true)]
        public static void TreatControlCAsInput(bool treatAsInput)
        {
            Console.TreatControlCAsInput = treatAsInput;
            ManualResetEvent mre = null;
            if (!treatAsInput)
            {
                mre = new ManualResetEvent(false);
                Console.CancelKeyPress += (_, args) =>
                {
                    Assert.Equal(ConsoleSpecialKey.ControlC, args.SpecialKey);
                    mre.Set();
                    args.Cancel = true;
                };
            }
            Console.WriteLine($"This is a test of TreatControlCAsInput={treatAsInput}. Please type 'Ctrl+C':");
            if (treatAsInput)
            {
                ConsoleKeyInfo keyInfo = Console.ReadKey(true);
                Assert.Equal(ConsoleKey.C, keyInfo.Key);
                Assert.Equal(ConsoleModifiers.Control, keyInfo.Modifiers);
                Assert.Equal('c', keyInfo.KeyChar);
            }
            else
            {
                Assert.True(mre.WaitOne(TimeSpan.FromSeconds(30)));
                Assert.False(Console.KeyAvailable);
            }
            AssertUserExpectedResults("no characters were echoed.");
        }

I verified this test would pass using a plain console app.

@tmds
Copy link
Member Author

tmds commented Sep 18, 2020

I verified this test would pass using a plain console app.

Actually, this assert would fail:

            Assert.Equal('c', keyInfo.KeyChar);

KeyChar is (char)3.

@sandreenko sandreenko added area-System.Runtime.InteropServices and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Sep 18, 2020
@stephentoub
Copy link
Member

Thanks, @tmds! Not sure how to interpret your test comments; are you able to add one?

@ghost
Copy link

ghost commented Sep 18, 2020

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

@eiriktsarpalis
Copy link
Member

Should we backport to 5.0?

I verified this test would pass using a plain console app.

I think we definitely need to convert manual tests to a plain-old console app.

@stephentoub
Copy link
Member

Should we backport to 5.0?

Yes

@eiriktsarpalis
Copy link
Member

/backport to release/5.0-rc2

@github-actions
Copy link
Contributor

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/261322915

@tmds
Copy link
Member Author

tmds commented Sep 18, 2020

Not sure how to interpret your test comments; are you able to add one?

No, pressing 'Ctrl+C' causes the parent process to kill the test process.

@jeffhandley
Copy link
Member

Remaining check timeouts are unrelated.

@jeffhandley jeffhandley merged commit e74c773 into dotnet:master Sep 19, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Console.TreatControlCAsInput is backwards in .NET 5 RC.1
6 participants