Clean up ProjectSnapshotManager tests #10051
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The testing infrastructure contains no fewer than six TestProjectSnapshotManager classes. This change merges five of them into one and converts the sixth into a mock.
Adds a TestProjectSnapshotManager.GetAccessor() method that produces an IProjectSnapshotManagerAccessor. This is used to clean up a lot of duplicated mocking code.
This change removes all of the extraneous constructors and the Create method from TestProjectSnapshotManager. In their place, CreateProjectSnapshotManager helpers have been added to the various tooling test fixture base classes.
This change makes strides toward removing mocks of ProjectSnapshotManagerBase. The biggest changes are in RazorProjectServiceTest, which has been rewritten to use real Razor objects and test outputs rather than mocking bits and pieces and verifying that members are called. While working on RazorProjectServiceTest.cs, I noticed a method exposed for tests that was only ever called by tests, so I removed that.
davidwengier
approved these changes
Mar 8, 2024
...t/Microsoft.AspNetCore.Razor.Test.Common.Tooling/ProjectSystem/TestProjectSnapshotManager.cs
Outdated
Show resolved
Hide resolved
...t.VisualStudio.LanguageServices.Razor.Test/LiveShare/Host/ProjectSnapshotManagerProxyTest.cs
Outdated
Show resolved
Hide resolved
…ager This addresses code review feedback from @davidwengier to fix the test data in ProjectSnapshotManagerProxyTest to avoid having multiple projects with identical intermediate output paths. In addition, I've rewritten these tests to use an TestProjectSnapshotManager rather than mocking IProjectSnapshotManager.
Thanks to @davidwengier for pointing out that this method can cause tests to be written in a way that hides bugs.
…ions This change adds TestProjectSnapshotManager.ListenToNotifications(), which makes it easy for test code to listen for project snapshot manager changes and assert the results.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To facilitate upcoming changes, this pull request audits and cleans up tooling tests that make use of a
ProjectSnapshotManager. There is now a singleTestProjectSnapshotManagerand a unified way to create one. I've also greatly reduced the number of mocks ofProjectSnapshotManagerBase, which will ultimately be going away. In particular, I've rewrittenRazorProjectServiceTest, which previously mocked various Razor tooling methods and verified that they were called properly. Because this sort of testing strategy can be particularly brittle when making large changes, so I've rewritten those tests to just use Razor tooling objects and test their outputs. In general, I hope that these will be easier to understand and maintain.While working
RazorProjectServiceTest, I noticed a method that had been made internal for tests, but was actually only ever called by tests. I've gotten rid of that bit.Recommend reviewing commit by commit.