Fix handful of unit test issues and the intermittent IndexOutOfRangeException failures #10635
Conversation
The ProcessWorkAndRestart test was throwing an exception and swallowing it because a document was added to a project when the document's file path was outside of the project's.
| 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. | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm trying to trace down when this exception would actually get caught. would you please illustrate a sample use case this catches?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
It's probably easiest to go through this commit-by-commit.
The main fix here is the
IndexOutOfRangeExceptionin theFilePathNormalizer. The problem occurs when the array rented from the pool already contains a\or/immediately after the span that we care about. It was possible forFilePathNormalizer.NormalizeAndDedupeSlashes(...)to read past the end of the span, and if it found a\or/there, it would cause the incorrect length to be used later, causing anIndexOutOfRangeException. The reason the failure was intermittent is because it's dependent on the contents of the array pool.