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

Fix handful of unit test issues and the intermittent IndexOutOfRangeException failures #10635

Merged
merged 7 commits into from
Jul 17, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,6 @@ public void Log(LogLevel logLevel, string message, Exception? exception)
{
_provider.TestOutputHelper.WriteLine(finalMessage);
}
catch (InvalidOperationException iex) when (iex.Message == "There is no currently active test.")
{
// Ignore, something is logging a message outside of a test. Other loggers will capture it.
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty sure this will break integration tests, where the VS instance is reused so the test logger definitely outlives a single test.

Could pass in a flag for whether to ignore these?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, interesting. I was wondering why this had been done because it was hiding other unit test failures in my investigation. I'll take a look at the integration tests.Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the way integration tests work with the test loggers, I'm not expecting this to break integration tests, though I can see a potential race condition. Running a handful of tests locally doesn't seem to be a problem. I'll try an integration test run here.

Copy link
Member

@maryamariyan maryamariyan Jul 17, 2024

Choose a reason for hiding this comment

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

I'm trying to trace down when this exception would actually get caught. would you please illustrate a sample use case this catches?

Copy link
Member Author

Choose a reason for hiding this comment

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

Integration tests passed just fine. I'm going to go ahead and merge this PR to get the intermittent test failure fixed. I'll address the race condition I spotted in a follow up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to trace down when this exception would actually get caught. would you please illustrate a sample use case this catches?

In the past, integration tests ran by starting VS, and then running each test by reusing the same VS instance. This meant there was a single instance of all of our VS components running across multiple individual tests. So it was possible for the first test to set up its specific ITestOutputHelper as a log target, finish running, but then the test output helper could continue to receive log messages from parts of Razor after the test had finished (eg, logs written during solution close, or background processing of something).

@DustinCampbell I had a quick look just now too, and I think perhaps with the logging infra changes this was fixed. It's possible that our previous code didn't clear the test output helper from the logger.

catch (Exception ex)
{
// If an exception is thrown while writing a message, throw an AggregateException that includes
Expand Down