Fix #1074 #1075
Fix #1074 #1075
Conversation
…source generators available
There was a problem hiding this comment.
The generated trees are already available. I use them in microsoft/vs-extension-testing for record/replay testing:
https://github.com/microsoft/vs-extension-testing/blob/b78ac2a63a471cc0a56b45af0a1d7b2473288202/src/Microsoft.VisualStudio.Extensibility.Testing.SourceGenerator.UnitTests/Verifiers/CSharpSourceGeneratorVerifier%601%2BTest.cs#L76-L80
| /// <summary> | ||
| /// Gets the syntax trees generated by the source generators during the test run. | ||
| /// </summary> | ||
| public ImmutableArray<SyntaxTree> GeneratedTrees { get; private set; } = ImmutableArray<SyntaxTree>.Empty; |
There was a problem hiding this comment.
📝 I have quite a few concerns about this. However, since no changes are required to implement the original feature, it might be easiest to just close this PR.
There was a problem hiding this comment.
See my comments in #1074. It should not be necessary to rewrite half of the test framework again and again, just to get hold of this simple thing.
There was a problem hiding this comment.
BTW, what are your concerns here?
| /// <summary> | ||
| /// Gets the syntax trees generated by the source generators during the test run. | ||
| /// </summary> | ||
| public ImmutableArray<SyntaxTree> GeneratedTrees { get; private set; } = ImmutableArray<SyntaxTree>.Empty; |
There was a problem hiding this comment.
❗ Source generators can run more than once during a test. It's not clear if/when/how this property would be valid during the life of a test. I would expect the following conceptual changes:
- The state of
AnalyzerTest<T>should not change during the life of a call toRunAsync. - The
GeneratedTreesshould not appear as a property onAnalyzerTest<T>, since that would make initializing the test instance more confusing (the output property would be showing up as an input property). - The manner in which generated trees are provided accounts for the fact that generators may run multiple times during a test.
There was a problem hiding this comment.
about 1:
it already does:
private readonly ConcurrentBag _workspaces = new ConcurrentBag()
about 2:
It won't, because the setter is private
about 3:
The sample you provided overwrites GetProjectCompilationAsync() to get access to the generated sources - but isn't that called also many times, so the problem is exactly the same?
There was a problem hiding this comment.
Maybe wrapping the call to GenerateSources into a new virtual method could be a good solution?
It would simplify extracting the sources a lot
There was a problem hiding this comment.
about 1:
it already does:
private readonly ConcurrentBag _workspaces = new ConcurrentBag()
This is true, but also specifically designed to account for an unknown number of workspace creations. This was a bit clumsy in its own addition, and hopefully it could be avoided for future cases.
about 2: It won't, because the setter is private
TestState doesn't have a setter at all. The syntax used to initialize TestState is also valid for properties that do not have a public setter.
about 3: The sample you provided overwrites
GetProjectCompilationAsync()to get access to the generated sources - but isn't that called also many times, so the problem is exactly the same?
It is, but the path that accesses the generated sources is only executed during specific record cases.
Maybe wrapping the call to GenerateSources into a new virtual method could be a good solution?
It would simplify extracting the sources a lot
ApplySourceGeneratorAsync might be the method to expose. The part I don't like is it's a difficult method to override.
|
Suggested solution works with acceptable effort |
Fix #1074: AnalyzerTesting: Make the generated syntax trees of source generators available