RazorProjectService cleanup #10989
RazorProjectService cleanup #10989
Conversation
Purely mechanical changes
Mostly these didn't test real scenarios, and one needed changing to test the UpdatedAsync method properly
| string? displayName, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| var service = instance; |
There was a problem hiding this comment.
Short answer: it makes the compiler happy
Long answer: The test accessor is a struct, and a lambda in a struct can't access a member of the struct. Or at least that's what I took the error to mean when the compiler complained at me :)
There was a problem hiding this comment.
ah interesting, so this is causing it to box? Will have to look more, but at first glance it was weird. Newfangled C#
There was a problem hiding this comment.
Yes, it would have to box the instance, and I guess they want that to be explicit and not hidden. I don't begin to understand this low level stuff :)
The error was CS1673 IIRC and you wanted to look up more about it.
There was a problem hiding this comment.
Yes, this is a problem because you're capturing a primary constructor parameter that is ultimately backed by a field on the struct. So, you're really capturing an instance field which means that the lambda must capture this, which requires boxing the struct. Otherwise, the closure would contain a copy of the struct.
There was a problem hiding this comment.
Here are the two errors @davidwengier likely ran into:
There was a problem hiding this comment.
Yea, this definitely makes sense. In a PR it looks weird but wholistically in the language is 👍
| @@ -956,9 +918,9 @@ public async Task UpdateDocument_ChangesDocumentInAllOwnerProjects() | |||
| const string RootNamespace = "TestRootNamespace"; | |||
| const string DocumentFilePath = "C:/path/to/document.cshtml"; | |||
|
|
|||
| var ownerProjectKey1 = await _projectService.AddProjectAsync( | |||
| var ownerProjectKey1 = await _projectService.GetTestAccessor().AddProjectAsync( | |||
There was a problem hiding this comment.
💭 should we just create a member for the test accessor?
There was a problem hiding this comment.
I'm not sure if I understood your suggestion correctly, but IMO we should actively get away from having members on production code "for testing".
There was a problem hiding this comment.
I meant in the RazorProjectServiceTest. It already is using a member variable for _projectService so I just assumed (without reading the full class) that allocating a new TestAccessor is just waisted typing becuase the underlying object being tested will always be _projectService
There was a problem hiding this comment.
I kinda like how the methods being called to set up test data look distinctly different in the tests. I also don't care about saving allocations of test accessors, or anything else in tests really (which I know some people definitely don't agree with 😛)
There was a problem hiding this comment.
allocations aren't my problem. I'm lazy and like typing fewer things. It's already here though let's stick with it :)
There was a problem hiding this comment.
Laziness wins (by me not changing the code 😛)
Noticed while working on my previous PR. There were a few methods in RazorProjectService that were only used by tests, and also resulted in some tests validating things that could never happen in the product (like passing
nullfor a project file path in an update). This PR removes one unused method, moves one to a test accessor where it rightly should be, and updates the tests that were callingUpdateProjectAsyncto instead call throughIRazorProjectServiceListener.UpdatedAsync.I don't love that these tests have to call through that interface, but I didn't change the method to be implicitly implemented because I don't think the name makes sense when written that way.
There are, or at least should be, no functionality changes in this PR.