Skip to content

Fixes #4804. Unit Tests are not running in degraded mode#4805

Closed
BDisp wants to merge 24 commits intogui-cs:v2_developfrom
BDisp:v2_4804_readfile-hang-fix
Closed

Fixes #4804. Unit Tests are not running in degraded mode#4805
BDisp wants to merge 24 commits intogui-cs:v2_developfrom
BDisp:v2_4804_readfile-hang-fix

Conversation

@BDisp
Copy link
Copy Markdown
Collaborator

@BDisp BDisp commented Mar 6, 2026

Fixes

The main reason this problem is occurring is because the unit tests are not running in degraded mode, since with xunit.v3 an executable file is created to perform the tests (.exe on Windows and without an extension on Linux and Mac).

Proposed Changes/Todos

  • Add cancelation to the wait call.

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@BDisp BDisp requested a review from tig as a code owner March 6, 2026 20:00
@BDisp BDisp marked this pull request as draft March 6, 2026 20:18
@BDisp
Copy link
Copy Markdown
Collaborator Author

BDisp commented Mar 7, 2026

In both VS2026 and VSCode, all unit tests pass. I think it has to do with the fact that the CI pipelines aren't running on a real console like on our machines, because the tests actually need to read the stream. I have no idea.

@tig
Copy link
Copy Markdown
Collaborator

tig commented Mar 7, 2026

This only happens on the windows runner, right?

@BDisp
Copy link
Copy Markdown
Collaborator Author

BDisp commented Mar 7, 2026

windows runner,

Yes, the crash only happens in the Windows runner. All the native Windows, Unix, and .NET functions (Console.IsInputRedirected || Console.IsOutputRedirected) return true if unit tests are being executed in their own environment, therefore they are never in degraded mode. If the unit tests are run on our own machines they pass, but if they are run in the CI pipeline tests they fail. I assume this is because they are not effectively connected to a terminal with a keyboard. Given this, it seems that checking if the input or output is not redirected is no longer reliable. If the Read method of each driver should not be executed in the unit tests, then another solution must be considered, although in the Unix system Read does not block, only the ReadFile in Windows.

Comment thread Terminal.Gui/App/Application.cs Outdated
/// <returns>
/// <see langword="true"/> if the test execution flag is set; otherwise, <see langword="false"/>.
/// </returns>
public static bool IsRunningInTest () => AppContext.TryGetSwitch ("Runtime.IsTestProject", out bool isTest) && isTest;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd much prefer this to be on IDriver. I worked hard to ensure there was no coupling from drivers to IApplication and this undoes that.

Can you refactor this such that it's a property on IDriver instead?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I was just testing and I really thought it wasn't the proper way. So I'll include it in IDriver as a property and implement it in DriverImpl. Does that sound good to you?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thinking about it, this method isn't characteristic of IDriver because it refers to the type of application being executed. I think it's better to add it as a property in IApplication and implement it in ApplicationImpl. I think that would be the right way. What do you think?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sorry. Forget my previous comment. The reason it's included in IDriver is that practically all the classes that use it are also used by IDriver. So you were right in your reasoning. Thanks.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that besides creating a property in IDriver and implementing it in DriverImpl, I'll also have to create the same property in IInput and IOutput. Don't you think that's too much work for just one property, which could even be static because when one test is running, all the others will run as well? I await your opinion and will work on the flaws in Linux and Mac.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Because it's also used in several places. See the image below. If the IInput and IOutput interfaces had the IDriver property, then in their implementations it would be enough to call something like if (Driver.IsRunningInTest). Then you also have the case of the static class ClipboardProcessRunner, which adds even more complexity. The UnixTerminalHelper class would be easier because it's only called by AnsiOutput and UnixOutput, and you would just need to add a new parameter bool isRunningInTest to its Suspend method. The most complicated to solve would be ClipboardProcessRunner because it doesn't have any reference to IDriver, IInput, and IOutput. With this explanation, see if you can find the best solution.

image

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I thought this only impacts windows.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I thought this only impacts windows.

That was my initial perception, but after the tests failed with all the drivers, I verified that all the low-level reading functions were actually being executed.
Unfortunately, I still don't understand why the tests are still failing. The API could actually be used without the need for Task.Delay, but rather by signaling an event. Now we just need to figure out where.

Copy link
Copy Markdown
Collaborator

@tig tig Mar 7, 2026

Choose a reason for hiding this comment

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

Try this: set an env variable in the yml. Have the IInput impls check for that variable.

There may already be an env variable set we can use.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Try this: set an env variable in the yml. Have the IInput impls check for that variable.

There may already be an env variable set we can use.

But the error is no longer due to low-level functions because they no longer execute with the configuration in the test projects. The problem now is that not all injected keys have yet been processed by the input processor.

@tig
Copy link
Copy Markdown
Collaborator

tig commented Mar 7, 2026

Ok. I'll shut up until you make progress on narrowing down the problem. It does seem odd to me though that before the work in this PR only the Windows runner was failing. Now all of them are.

Thanks for continuing to work on this. Let me know how/if I can help.

@tig
Copy link
Copy Markdown
Collaborator

tig commented Mar 7, 2026

JFYI: https://xunit.net/docs/capturing-output

We should NOT use this btw. But it does point to xunit3 wrapping console output. It's possible they also wrap console input or the new arch somehow impacts it. I stand by my suggestion that we have the drivers check for an evn var at initalization and disable real read/write paths if detected (degraded mode).

The best and officially recommended environment variable defined by GitHub to detect that your code (e.g., tests, build scripts, or app logic) is running inside a GitHub Actions workflow is:
GITHUB_ACTIONS

Value: Always set to "true" (as a string) when the workflow is executing on a GitHub Actions runner.
Behavior: It's not present (or empty/null) when running locally (e.g., dotnet test on your machine, VS debug, etc.).

Why it's the top choice:

It's explicitly documented by GitHub for this exact purpose: to differentiate between local runs and CI runs in GitHub Actions.
From the official docs (Variables reference): "GITHUB_ACTIONS: Always set to true when GitHub Actions is running the workflow. You can use this variable to differentiate when tests are being run locally or by GitHub Actions."
It's reliable, process-wide (available to every step), and can't be overwritten (GitHub protects GITHUB_* variables from modification).
It's more specific than the generic CI=true (which many other CI systems like CircleCI, Travis, GitLab CI, etc., also set).

bool IsGitHubActions = Environment.GetEnvironmentVariable("GITHUB_ACTIONS") == "true";

@BDisp
Copy link
Copy Markdown
Collaborator Author

BDisp commented Mar 7, 2026

I don't understand how your explanation solves this specific case. Locally, the low-level methods were being executed when they shouldn't be. By configuring the unit test files and creating the DriverImpl.IsRunningInTest property, this problem was resolved, and now the tests don't fail locally (degraded mode). What I find strange is that CI pipelines are also running in degraded mode and still failing, sometimes Windows and Mac passed, but Ubuntu always failed.

I think I'll give up on this and close the PR, what do you think?

@tig
Copy link
Copy Markdown
Collaborator

tig commented Mar 8, 2026

I see. You are trying to solve two related but separate issues here. I am just focusing on the gh runner issue.

However I think my suggestion could be useful for both: when running tests the drivers should NEVER use the real read/write path. Thus, just set an environment variable in the test .csproj that the driver Init code checks.

@BDisp
Copy link
Copy Markdown
Collaborator Author

BDisp commented Mar 8, 2026

I see. You are trying to solve two related but separate issues here. I am just focusing on the gh runner issue.

However I think my suggestion could be useful for both: when running tests the drivers should NEVER use the real read/write path. Thus, just set an environment variable in the test .csproj that the driver Init code checks.

I added the following configuration to all test projects, which is still more advisable than using environment variables because it's AOT compatible.

	<ItemGroup>
		<RuntimeHostConfigurationOption
			Include="Runtime.IsTestProject"
			Value="true"
			Trim="false" />
	</ItemGroup>

I also added the following property to access the project settings. Do you think that's bad?

    public static bool IsRunningInTest => AppContext.TryGetSwitch ("Runtime.IsTestProject", out bool isTest) && isTest;

@tig
Copy link
Copy Markdown
Collaborator

tig commented Mar 8, 2026

I think we learned last time we had an "IsRunningTests" flag that the name was too broad and ended up being confusing.

I suggest a name that's very specific to how it's used: DisableDriverRealIO.

@BDisp
Copy link
Copy Markdown
Collaborator Author

BDisp commented Mar 8, 2026

I think we learned last time we had an "IsRunningTests" flag that the name was too broad and ended up being confusing.

I suggest a name that's very specific to how it's used: DisableDriverRealIO.

Done in 4d97378.

@tig
Copy link
Copy Markdown
Collaborator

tig commented Mar 8, 2026

Now don't you need to revert all the other chanaes you made here that are no actually needed?

@BDisp BDisp changed the title Fixes #4804. InjectKey Tests with Pipeline Mode hangs forever in the ReadFile or XunitAutoGeneratedEntryPoint Fixes #4804. Unit Tests are not running in degraded mode Mar 8, 2026
@BDisp
Copy link
Copy Markdown
Collaborator Author

BDisp commented Mar 8, 2026

Now don't you need to revert all the other chanaes you made here that are no actually needed?

I've changed the titles of the issue and the fix because the CI pipeline tests are still failing, and I'm giving up. If there's anything else to fix, please let me know.

@BDisp BDisp marked this pull request as ready for review March 8, 2026 20:29
@tig
Copy link
Copy Markdown
Collaborator

tig commented Mar 8, 2026

@BDisp I need a fix for this for #4810 and am in the middle of implementing a simple and robust solution that ensures that drivers never try to use real input or output when running in tests. I'll share it asap.

@tig tig closed this Mar 9, 2026
@BDisp BDisp deleted the v2_4804_readfile-hang-fix branch March 9, 2026 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unit Tests are not running in degraded mode

2 participants