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.

Sign up for GitHub

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

Jump to bottom

Progress indicator. #1964

Merged

singhsarab merged 11 commits into microsoft:master from singhsarab:ProgressIndicator

Mar 26, 2019

Contributor

singhsarab commented Mar 20, 2019

Description

Adding the code for indicating that the run is in progress.

Related issue


          Draft for the progress indicator.

eae3b89

singhsarab requested a review from abhishkk

March 20, 2019 20:04

singhsarab added 2 commits

March 22, 2019 20:15


          Swtich to timer for showing progress.

c734db1


          Adding NoOpProgressIndicator and unit tests.

5097f29

singhsarab marked this pull request as ready for review

March 22, 2019 18:36

singhsarab added 2 commits

March 25, 2019 16:29


          Remove tests that would not run in CI

ca8af38


          Merge branch 'master' into ProgressIndicator

84afbae

singhsarab changed the title ~~Draft for the progress indicator.~~ Progress indicator.

abhishkk reviewed

View reviewed changes

src/vstest.console/Internal/ConsoleLogger.cs Outdated

                           ValidateArg.NotNull<object>(sender, "sender");
                           ValidateArg.NotNull<TestRunMessageEventArgs>(e, "e");
+                          this.progressIndicator.Pause();

Contributor

abhishkk Mar 25, 2019

Can we add a note here?

Contributor Author

singhsarab Mar 25, 2019

done.

abhishkk reviewed

View reviewed changes

src/vstest.console/Internal/ConsoleLogger.cs Outdated

+                              // Use the no operation progress indicator is the standard out is getting redirected.
+                              if (Console.IsOutputRedirected)
+                              {
+                                  this.progressIndicator = new NoOpProgressIndicator();

Contributor

abhishkk Mar 25, 2019

As NoOpProgressIndicator implementation is empty, we can initialize this.progressIndicator only when console output is not redirected. In other case, it will be null.

Now anywhere we are using this.progressIndicator.Pause();, we can do this.progressIndicator?.Pause();

abhishkk reviewed

View reviewed changes

src/vstest.console/Internal/ProgressIndicator/NoOpProgressIndicator.cs Outdated

+              namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Internal
+              {
+                  internal class NoOpProgressIndicator : IProgressIndicator

Contributor

abhishkk Mar 25, 2019

In case we are removing NoOpProgressIndicator, we can remove interface as well.

Contributor Author

singhsarab Mar 25, 2019

Need it for testing.

abhishkk reviewed

View reviewed changes

src/vstest.console/Internal/ProgressIndicator/ProgressIndicator.cs Outdated

+              namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Internal
+              {
+                  using Microsoft.VisualStudio.TestPlatform.Utilities;

Contributor

abhishkk Mar 25, 2019 •

edited

Loading

nit: system using sort

abhishkk reviewed

View reviewed changes

src/vstest.console/Internal/ProgressIndicator/ProgressIndicator.cs Outdated

+                  using System;
+                  using Timer = System.Timers.Timer;
+                  public class ProgressIndicator : IProgressIndicator

Contributor

abhishkk Mar 25, 2019

we can make class internal

abhishkk reviewed

View reviewed changes

src/vstest.console/Internal/ProgressIndicator/ProgressIndicator.cs Outdated

+                      {
+                          lock (syncObject)
+                          {
+                              if (timer == null)

Contributor

abhishkk Mar 25, 2019

we can make timer private property and move these 3-4 line to getter.

abhishkk reviewed

View reviewed changes

src/vstest.console/Internal/ProgressIndicator/ProgressIndicator.cs Outdated

+                                  timer.Start();
+                              }
+                              startTime = DateTime.Now;
+                              ConsoleOutput.Write(testRunProgressString.Substring(0, testRunProgressString.Length + dotCounter - 2), OutputLevel.Information);

Contributor

abhishkk Mar 25, 2019

Can we write a note here why we are doing substr?

abhishkk reviewed

View reviewed changes

src/vstest.console/Internal/ProgressIndicator/ProgressIndicator.cs Outdated

+                          var currentLineCursor = Console.CursorTop;
+                          Console.SetCursorPosition(startPos, Console.CursorTop);
+                          ConsoleOutput.Write(new string(' ', Console.WindowWidth - startPos), OutputLevel.Information);
+                          Console.SetCursorPosition(startPos, currentLineCursor);

Contributor

abhishkk Mar 25, 2019

we can add note here as well

abhishkk reviewed

View reviewed changes

src/vstest.console/Internal/ProgressIndicator/ProgressIndicator.cs Outdated

+                          {
+                              ConsoleOutput.Write(".", OutputLevel.Information);
+                              dotCounter = ++dotCounter % 3;
+                              if (dotCounter == 0)

Contributor

abhishkk Mar 25, 2019

Lets add note for this scenario as well

abhishkk reviewed

View reviewed changes

src/vstest.console/Internal/ProgressIndicator/ProgressIndicator.cs Outdated

+                          Console.SetCursorPosition(startPos, currentLineCursor);
+                      }
+                      public void Pause()

Contributor

abhishkk Mar 25, 2019

To test pause, start, stop, can we create consoleWrapper so as to check functionalities like:

pause is pausing the indicator
pause is clearing/setting the cursor point
stop is stopping the timer
etc.

microsoft deleted a comment from abhishkk


          Adding tests and reacting based on the comments.

63b0138

microsoft deleted a comment from abhishkk

singhsarab added 3 commits

March 25, 2019 18:30


          Merge branch 'ProgressIndicator' of https://github.com/singhsarab/vstest

d810b03

 into ProgressIndicator


          Adding the resource files and docs on some missing methods.

1fba5bb


          Fixing the resource

dacc020

abhishkk approved these changes

View reviewed changes

Contributor

abhishkk left a comment

We need to regenerate resources again.

src/vstest.console/Internal/ConsoleLogger.cs Outdated

+                          if (this.progressIndicator == null && !Console.IsOutputRedirected)
+                          {
+                              // Progress indicator needs to be displayed only for cli experience.
+                              // Use the no operation progress indicator is the standard out is getting redirected.

Contributor

abhishkk Mar 26, 2019

nit: line comment seems incorrect

src/vstest.console/Internal/Interfaces/IConsoleHelper.cs Outdated

+                  /// <summary>
+                  /// Interface for wrapping the Console class
+                  /// </summary>
+                  public interface IConsoleHelper

Contributor

abhishkk Mar 26, 2019

nit: internal interface

src/vstest.console/Internal/Interfaces/IProgressIndicator.cs Outdated

+              namespace Microsoft.VisualStudio.TestPlatform.CommandLine.Internal
+              {
+                  public interface IProgressIndicator

Contributor

abhishkk Mar 26, 2019

nit: internal interface

src/vstest.console/Internal/ProgressIndicator.cs Outdated

+                          lock (syncObject)
+                          {
+                              IsRunning = false;
+                              Clear(0);

Contributor

abhishkk Mar 26, 2019

nit: this.Clear(0)

src/vstest.console/Resources/xlf/Resources.cs.xlf Outdated

@@ @@ -1637,6 +1637,11 @@ @@
                       <target state="new">Zadání Framework35 není podporováno. U projektů mířících na .Net Framework 3.5 prosím použijte Framework40, aby testy v CLR 4.0 běžely v režimu kompatibility.</target>
                       <note></note>
                     </trans-unit>
+                    <trans-unit id="ProgressIndicatorString">
+                      <source>Test run in progress</source>
+                      <target state="new">Test run in progress...</target>

Contributor

abhishkk Mar 26, 2019

we need to re-generate the resources. It's still picking old value

singhsarab added 2 commits

March 26, 2019 11:56


          Fixed the xlfs and small changes as per comments.

0bdc466


          Adding comments

f3330a6

singhsarab merged commit 30bf95c into microsoft:master

nguerrera mentioned this pull request

Massive xunit test performance degradation in .NET core 2.1.700 and 2.2.300 dotnet/core#2775

Closed

jorgensigvardsson commented Jul 1, 2019

So, has this change surfaced to the public yet? Running 2.2.204 SDK docker image for now. Should I keep on doing that until 3.0 is out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet