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.

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

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

Merged
merged 7 commits into from
Jul 17, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e67fab8
Add missing `ReturnAsync` for mock
DustinCampbell Jul 16, 2024
2edc201
Move CreateTextLoader helper to TestMocks class
DustinCampbell Jul 16, 2024
d96c004
Fix BackgroundDocumentGeneratorTest that was throwing exception
DustinCampbell Jul 16, 2024
3138fb0
Do not swallow test logging errors that occur outside of tests
DustinCampbell Jul 16, 2024
214d921
Ensure that we log exceptions during tests
DustinCampbell Jul 16, 2024
59aa56d
Don't read past end of span in NormalizeAndDedupeSlashes
DustinCampbell Jul 17, 2024
6d76c6d
Add "/" as special case when normalizing directories
DustinCampbell Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions ...rosoft.AspNetCore.Razor.LanguageServer.Test/Completion/TagHelperTooltipFactoryBaseTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Microsoft.AspNetCore.Razor.Language.Components;
using Microsoft.AspNetCore.Razor.LanguageServer.Tooltip;
using Microsoft.AspNetCore.Razor.ProjectSystem;
using Microsoft.AspNetCore.Razor.Test.Common;
using Microsoft.AspNetCore.Razor.Test.Common.LanguageServer;
using Microsoft.CodeAnalysis.Razor.ProjectSystem;
using Roslyn.Test.Utilities;
Expand Down Expand Up @@ -374,7 +375,7 @@ await projectManager.UpdateAsync(updater =>
{
updater.ProjectAdded(hostProject);
updater.ProjectWorkspaceStateChanged(hostProject.Key, projectWorkspaceState);
updater.DocumentAdded(hostProject.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
updater.DocumentAdded(hostProject.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));
});

var service = new TestTagHelperToolTipFactory(projectManager);
Expand Down Expand Up @@ -419,11 +420,11 @@ await projectManager.UpdateAsync(updater =>
{
updater.ProjectAdded(hostProject1);
updater.ProjectWorkspaceStateChanged(hostProject1.Key, projectWorkspaceState);
updater.DocumentAdded(hostProject1.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
updater.DocumentAdded(hostProject1.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));

updater.ProjectAdded(hostProject2);
updater.ProjectWorkspaceStateChanged(hostProject2.Key, projectWorkspaceState);
updater.DocumentAdded(hostProject2.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
updater.DocumentAdded(hostProject2.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));
});

var service = new TestTagHelperToolTipFactory(projectManager);
Expand Down Expand Up @@ -468,10 +469,10 @@ await projectManager.UpdateAsync(updater =>
{
updater.ProjectAdded(hostProject1);
updater.ProjectWorkspaceStateChanged(hostProject1.Key, projectWorkspaceState);
updater.DocumentAdded(hostProject1.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
updater.DocumentAdded(hostProject1.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));

updater.ProjectAdded(hostProject2);
updater.DocumentAdded(hostProject2.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
updater.DocumentAdded(hostProject2.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));
});

var service = new TestTagHelperToolTipFactory(projectManager);
Expand Down Expand Up @@ -512,10 +513,10 @@ public async Task GetAvailableProjects_NotAvailableInAnyProject_ReturnsText()
await projectManager.UpdateAsync(updater =>
{
updater.ProjectAdded(hostProject1);
updater.DocumentAdded(hostProject1.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
updater.DocumentAdded(hostProject1.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));

updater.ProjectAdded(hostProject2);
updater.DocumentAdded(hostProject2.Key, hostDocument, CreateTextLoader(hostDocument.FilePath, text: ""));
updater.DocumentAdded(hostProject2.Key, hostDocument, TestMocks.CreateTextLoader(hostDocument.FilePath, text: ""));
});

var service = new TestTagHelperToolTipFactory(projectManager);
Expand Down
6 changes: 3 additions & 3 deletions src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/DocumentContextFactoryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public async Task TryCreateForOpenDocumentAsync_CanNotResolveVersion_ReturnsNull

await _projectManager.UpdateAsync(updater =>
{
updater.DocumentAdded(MiscFilesHostProject.Instance.Key, hostDocument, CreateTextLoader(filePath, ""));
updater.DocumentAdded(MiscFilesHostProject.Instance.Key, hostDocument, TestMocks.CreateTextLoader(filePath, ""));
});

var factory = new DocumentContextFactory(_projectManager, _documentVersionCache, LoggerFactory);
Expand All @@ -87,7 +87,7 @@ public async Task TryCreateAsync_ResolvesContent()

await _projectManager.UpdateAsync(updater =>
{
updater.DocumentAdded(MiscFilesHostProject.Instance.Key, hostDocument, CreateTextLoader(filePath, ""));
updater.DocumentAdded(MiscFilesHostProject.Instance.Key, hostDocument, TestMocks.CreateTextLoader(filePath, ""));
});

var miscFilesProject = _projectManager.GetMiscellaneousProject();
Expand Down Expand Up @@ -142,7 +142,7 @@ public async Task TryCreateForOpenDocumentAsync_ResolvesContent()

await _projectManager.UpdateAsync(updater =>
{
updater.DocumentAdded(MiscFilesHostProject.Instance.Key, hostDocument, CreateTextLoader(filePath, ""));
updater.DocumentAdded(MiscFilesHostProject.Instance.Key, hostDocument, TestMocks.CreateTextLoader(filePath, ""));
});

var miscFilesProject = _projectManager.GetMiscellaneousProject();
Expand Down
18 changes: 0 additions & 18 deletions ...t/Microsoft.AspNetCore.Razor.Test.Common.Tooling/LanguageServer/LanguageServerTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
using Microsoft.CodeAnalysis.Text;
using Microsoft.CommonLanguageServerProtocol.Framework;
using Microsoft.VisualStudio.LanguageServer.Protocol;
using Moq;
using Xunit.Abstractions;

namespace Microsoft.AspNetCore.Razor.Test.Common.LanguageServer;
Expand Down Expand Up @@ -131,23 +130,6 @@ private protected static VersionedDocumentContext CreateDocumentContext(Uri uri,
return new VersionedDocumentContext(uri, snapshot, projectContext: null, version: 0);
}

protected static TextLoader CreateTextLoader(string filePath, string text)
{
return CreateTextLoader(filePath, SourceText.From(text));
}

protected static TextLoader CreateTextLoader(string filePath, SourceText text)
{
var mock = new StrictMock<TextLoader>();

var textAndVersion = TextAndVersion.Create(text, VersionStamp.Create(), filePath);

mock.Setup(x => x.LoadTextAndVersionAsync(It.IsAny<LoadTextOptions>(), It.IsAny<CancellationToken>()))
.ReturnsAsync(textAndVersion);

return mock.Object;
}

private protected static RazorLSPOptionsMonitor GetOptionsMonitor(bool enableFormatting = true, bool autoShowCompletion = true, bool autoListParams = true, bool formatOnType = true, bool autoInsertAttributeQuotes = true, bool colorBackground = false, bool codeBlockBraceOnNextLine = false, bool commitElementsWithSpace = true)
{
var configService = StrictMock.Of<IConfigurationSyncService>();
Expand Down
4 changes: 0 additions & 4 deletions src/Razor/test/Microsoft.AspNetCore.Razor.Test.Common.Tooling/Logging/TestOutputLogger.cs
View file
Open in desktop
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 •
edited
Loading

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
29 changes: 29 additions & 0 deletions src/Razor/test/Microsoft.AspNetCore.Razor.Test.Common.Tooling/TestMocks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root for license information.

using System.Threading;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Text;
using Moq;

namespace Microsoft.AspNetCore.Razor.Test.Common;

internal static class TestMocks
{
public static TextLoader CreateTextLoader(string filePath, string text)
{
return CreateTextLoader(filePath, SourceText.From(text));
}

public static TextLoader CreateTextLoader(string filePath, SourceText text)
{
var mock = new StrictMock<TextLoader>();

var textAndVersion = TextAndVersion.Create(text, VersionStamp.Create(), filePath);

mock.Setup(x => x.LoadTextAndVersionAsync(It.IsAny<LoadTextOptions>(), It.IsAny<CancellationToken>()))
.ReturnsAsync(textAndVersion);

return mock.Object;
}
}
32 changes: 20 additions & 12 deletions ...alStudio.LanguageServices.Razor.Test/DocumentGenerator/BackgroundDocumentGeneratorTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,17 @@
using Microsoft.AspNetCore.Razor.ProjectSystem;
using Microsoft.AspNetCore.Razor.Test.Common;
using Microsoft.AspNetCore.Razor.Test.Common.VisualStudio;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Razor;
using Microsoft.CodeAnalysis.Razor.Logging;
using Microsoft.CodeAnalysis.Razor.ProjectSystem;
using Microsoft.CodeAnalysis.Text;
using Microsoft.VisualStudio.Razor.DynamicFiles;
using Moq;
using Xunit;
using Xunit.Abstractions;

namespace Microsoft.CodeAnalysis.Razor.ProjectSystem;
namespace Microsoft.VisualStudio.Razor.ProjectSystem;

// These tests are really integration tests. There isn't a good way to unit test this functionality since
// the only thing in here is threading.
Expand Down Expand Up @@ -219,27 +222,32 @@ await projectManager.UpdateAsync(updater =>
[UIFact]
public async Task ProcessWorkAndRestart()
{
var hostProject1 = TestProjectData.SomeProject;
var hostProject2 = TestProjectData.AnotherProject;
var hostDocument1 = TestProjectData.SomeProjectFile1;
var hostDocument2 = TestProjectData.SomeProjectFile2;

// Arrange
var projectManager = CreateProjectSnapshotManager();

await projectManager.UpdateAsync(updater =>
{
updater.ProjectAdded(s_hostProject1);
updater.ProjectAdded(s_hostProject2);
updater.DocumentAdded(s_hostProject1.Key, s_documents[0], null!);
updater.DocumentAdded(s_hostProject1.Key, s_documents[1], null!);
updater.ProjectAdded(hostProject1);
updater.ProjectAdded(hostProject2);
updater.DocumentAdded(hostProject1.Key, hostDocument1, null!);
updater.DocumentAdded(hostProject1.Key, hostDocument2, null!);
});

var project = projectManager.GetLoadedProject(s_hostProject1.Key);
var documentKey1 = new DocumentKey(project.Key, s_documents[0].FilePath);
var documentKey2 = new DocumentKey(project.Key, s_documents[1].FilePath);
var project = projectManager.GetLoadedProject(hostProject1.Key);
var documentKey1 = new DocumentKey(project.Key, hostDocument1.FilePath);
var documentKey2 = new DocumentKey(project.Key, hostDocument2.FilePath);

using var generator = new TestBackgroundDocumentGenerator(projectManager, _dynamicFileInfoProvider, LoggerFactory);

// Act & Assert

// First, enqueue some work.
generator.Enqueue(project, project.GetDocument(s_documents[0].FilePath).AssumeNotNull());
generator.Enqueue(project, project.GetDocument(hostDocument1.FilePath).AssumeNotNull());

// Wait for the work to complete.
await generator.WaitUntilCurrentBatchCompletesAsync();
Expand All @@ -248,14 +256,14 @@ await projectManager.UpdateAsync(updater =>
Assert.Single(generator.CompletedWork, documentKey1);

// Enqueue more work.
generator.Enqueue(project, project.GetDocument(s_documents[1].FilePath).AssumeNotNull());
generator.Enqueue(project, project.GetDocument(hostDocument2.FilePath).AssumeNotNull());

// Wait for the work to complete.
await generator.WaitUntilCurrentBatchCompletesAsync();

Assert.Collection(generator.CompletedWork.OrderBy(key => key.DocumentFilePath),
key => Assert.Equal(documentKey2, key),
key => Assert.Equal(documentKey1, key));
key => Assert.Equal(documentKey1, key),
key => Assert.Equal(documentKey2, key));
}

[UIFact]
Expand Down
1 change: 1 addition & 0 deletions ...t.VisualStudio.LanguageServices.Razor.Test/Documents/VisualStudioFileChangeTrackerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ public async Task StopListening_UnadvisesForFileChange()
.Verifiable();
fileChangeService
.Setup(f => f.UnadviseFileChangeAsync(123, It.IsAny<CancellationToken>()))
.ReturnsAsync(TestProjectData.SomeProjectImportFile.FilePath)
.Verifiable();
var tracker = new VisualStudioFileChangeTracker(
TestProjectData.SomeProjectImportFile.FilePath,
Expand Down
13 changes: 7 additions & 6 deletions ...io.LanguageServices.Razor.Test/LanguageClient/ProjectSystem/RazorProjectInfoDriverTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.LanguageServer.ProjectSystem;
using Microsoft.AspNetCore.Razor.ProjectSystem;
using Microsoft.AspNetCore.Razor.Test.Common;
using Microsoft.AspNetCore.Razor.Test.Common.LanguageServer;
using Microsoft.CodeAnalysis.Razor.ProjectSystem;
using Xunit;
Expand Down Expand Up @@ -41,10 +42,10 @@ public async Task ProcessesExistingProjectsDuringInitialization()
await projectManager.UpdateAsync(static updater =>
{
updater.ProjectAdded(s_hostProject1);
updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, TestMocks.CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));

updater.ProjectAdded(s_hostProject2);
updater.DocumentAdded(s_hostProject2.Key, s_hostDocument2, CreateTextLoader(s_hostDocument2.FilePath, "<p>Hello World</p>"));
updater.DocumentAdded(s_hostProject2.Key, s_hostDocument2, TestMocks.CreateTextLoader(s_hostDocument2.FilePath, "<p>Hello World</p>"));
});

var (driver, testAccessor) = await CreateDriverAndInitializeAsync(projectManager);
Expand Down Expand Up @@ -93,10 +94,10 @@ public async Task ProcessesProjectsAddedAfterInitialization()
await projectManager.UpdateAsync(static updater =>
{
updater.ProjectAdded(s_hostProject1);
updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, TestMocks.CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));

updater.ProjectAdded(s_hostProject2);
updater.DocumentAdded(s_hostProject2.Key, s_hostDocument2, CreateTextLoader(s_hostDocument2.FilePath, "<p>Hello World</p>"));
updater.DocumentAdded(s_hostProject2.Key, s_hostDocument2, TestMocks.CreateTextLoader(s_hostDocument2.FilePath, "<p>Hello World</p>"));
});

await testAccessor.WaitUntilCurrentBatchCompletesAsync();
Expand Down Expand Up @@ -134,7 +135,7 @@ await projectManager.UpdateAsync(static updater =>

await projectManager.UpdateAsync(static updater =>
{
updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, TestMocks.CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
});

await testAccessor.WaitUntilCurrentBatchCompletesAsync();
Expand Down Expand Up @@ -204,7 +205,7 @@ await projectManager.UpdateAsync(static updater =>

await projectManager.UpdateAsync(static updater =>
{
updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
updater.DocumentAdded(s_hostProject1.Key, s_hostDocument1, TestMocks.CreateTextLoader(s_hostDocument1.FilePath, "<p>Hello World</p>"));
});

await testAccessor.WaitUntilCurrentBatchCompletesAsync();
Expand Down