Disable processing of source-generated documents in unit testing#82359
Conversation
| // include them in incremental analysis. | ||
| foreach (var document in await project.GetSourceGeneratedDocumentsAsync(_shutdownToken).ConfigureAwait(false)) | ||
| await EnqueueDocumentWorkItemAsync(project, document.Id, document, invocationReasons).ConfigureAwait(false); | ||
| if (processSourceGeneratedDocuments) |
There was a problem hiding this comment.
Here's the actual behavior change.
| private static void CheckStorageKeyAndType(string storageKey, [NotNull] Type? storageType) | ||
| { | ||
| Contract.ThrowIfFalse(storageKey.StartsWith("languages"), "Need to update SubscribeToChanges in constructor to listen to changes to this key"); | ||
| // We don't need to listen to changes to test.realTimeDiscovery.includeSourceGeneratedFiles because we require a restart anyways |
There was a problem hiding this comment.
Yes, this is a hack.
There was a problem hiding this comment.
So it looks like this is verifying that any option we read the value of, we also listen to changes for? Is it not an option to listen for changes, but not respect them?
Or otherwise it seems like the ability to read an option without listening to changes is something we should just in general (rather than this option only), as I'm sure we'll have other options in the future that also require restarts.
There was a problem hiding this comment.
When we move more stuff over to unified settings (and not rely on the migration), we'd enhance this generally to listen for any key we read. I didn't want to spend time building that when the only additional key (for now) is one we can't actually process changes for.
| private static void CheckStorageKeyAndType(string storageKey, [NotNull] Type? storageType) | ||
| { | ||
| Contract.ThrowIfFalse(storageKey.StartsWith("languages"), "Need to update SubscribeToChanges in constructor to listen to changes to this key"); | ||
| // We don't need to listen to changes to test.realTimeDiscovery.includeSourceGeneratedFiles because we require a restart anyways |
There was a problem hiding this comment.
So it looks like this is verifying that any option we read the value of, we also listen to changes for? Is it not an option to listen for changes, but not respect them?
Or otherwise it seems like the ability to read an option without listening to changes is something we should just in general (rather than this option only), as I'm sure we'll have other options in the future that also require restarts.
| "IsFreeThreaded"=dword:00000001 | ||
|
|
||
| // CacheTag value should be changed when registration file changes | ||
| // See https://devdiv.visualstudio.com/DevDiv/_wiki/wikis/DevDiv.wiki/39345/Manifest-Build-Deployment-and-Setup-Authoring-In-Depth?anchor=example-pkgdef-key for more infomation |
|
FYI there are whole testing frameworks designed for putting tests in Razor files: https://bunit.dev Don't know how it works with test explorer etc but thought I'd mentioned it in case this regresses that. |
|
@davidwengier I'd expect the only regression there would be source-based test discovery would no longer work. Discovery based on the last build would still work fine. That said, I'm not sure if it would work in that case, or at wouldn't have worked anyways prior to cohosting anyways. |
9335763 to
d942727
Compare
While investigating a report of ServiceHub consuming a large amount of memory, I noticed the unit testing solution crawler queue was quite large, and (at least at the point of the dump) it was running source generators to add the source generated files to the queue. When chatting with the unit testing team, it's not clear that's really a scenario we need to support in the first place, so as a performance test we'd like to simply disable that. This defines a new setting to disable this, and wires it up to a feature flag for us to experiment with this. The core code change is minimial -- most of this is defining a new Roslyn setting that's not language specific, and passing the boolean around.
d942727 to
148f754
Compare
While investigating a report of ServiceHub consuming a large amount of memory, I noticed the unit testing solution crawler queue was quite large, and (at least at the point of the dump) it was running source generators to add the source generated files to the queue. When chatting with the unit testing team, it's not clear that's really a scenario we need to support in the first place, so as a performance test we'd like to simply disable that.
This defines a new setting to disable this, and wires it up to a feature flag for us to experiment with this. The core code change is minimial -- most of this is defining a new roslyn setting that's not language specific, and passing the boolean around.