Skip to content
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

Interactive display for terminals #3292

Merged
merged 38 commits into from
Aug 9, 2024
Merged

Interactive display for terminals #3292

merged 38 commits into from
Aug 9, 2024

Conversation

nohwnd
Copy link
Member

@nohwnd nohwnd commented Jul 22, 2024

Adds output device that writes tests to ansi or non-ansi terminal, with or without progress indicator.

Features:

A "progress" bar that shows counts of passed, failed and skipped tests. In ANSI mode the progress is updated live on the bottom of the screen, in non-ansi mode the progress is output repeatedly every 3 seconds.

image

Colored stack traces, with links to files, and (optional) relative paths:

image

Test run summary:

image

artifacts report:

image

Optionally showing passed tests:

image

Optionally showing link to assembly the test is coming from:

image

image

Optional per assembly summary:
image

  • implement TestHostControllerInfo.CurrentProcessIsTestHostController to disable progress reporting in controller.
  • fix output tests

Split:

@nohwnd
Copy link
Member Author

nohwnd commented Jul 23, 2024

failed test, failed summary:
image

progress indicator:

image

optional output of passed tests, and optional source of the message (will be useful for dotnet test):

image

optional assembly summary:

image

attachements:

image

busy indicator:

image

@nohwnd nohwnd marked this pull request as ready for review July 23, 2024 13:01
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

I need more time but sending my first comments

src/Platform/Microsoft.Testing.Platform/UI/AnsiCodes.cs Outdated Show resolved Hide resolved
src/Platform/Microsoft.Testing.Platform/UI/ITerminal.cs Outdated Show resolved Hide resolved
src/Platform/Microsoft.Testing.Platform/UI/AnsiTerminal.cs Outdated Show resolved Hide resolved
src/Platform/Microsoft.Testing.Platform/UI/AnsiTerminal.cs Outdated Show resolved Hide resolved
src/Platform/Microsoft.Testing.Platform/UI/IMessage.cs Outdated Show resolved Hide resolved
_consoleWithProgress = consoleWithProgress;
}

public void TestExecutionStarted(DateTimeOffset testStartTime, int workerCount)
Copy link
Member

Choose a reason for hiding this comment

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

It's called from only one place with 1 for worker, do we need the parameter? What does it represent?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be used by dotnet test when we have multiple projects running via this same logger, when there is the new protocol between dotnet test and test .exe.

Copy link
Member

Choose a reason for hiding this comment

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

Is it responsibility of the caller or this class to know how many state object should handle the progress?

Copy link
Member Author

Choose a reason for hiding this comment

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

The caller tells us what is the level of parallelism, so we can know how many projects will run at the same time at max, and make UI choices based on that. e.g. we could write: "Starting run in parallel on 10 workers".

Actually managing the states is the responsibility of this console with progress.

Comment on lines +27 to +29
public List<string> Attachments { get; } = new();

public List<IProgressMessage> Messages { get; } = new();
Copy link
Member

Choose a reason for hiding this comment

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

Could we expose as "readonly"/"immutable" to avoid having anything that can mutate this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be done, but it feels like too much ceremony for this simple internal object that just holds the state and is not passed outside of the logger.

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

We still need to get rid of all the logger wording

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Merging as-is. We will do a follow-up for the last few nits

@Evangelink Evangelink merged commit fac90cc into main Aug 9, 2024
6 checks passed
@Evangelink Evangelink deleted the interactive-logger branch August 9, 2024 06:32
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.

3 participants