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.

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

Add an experimental API for testing incremental steps. #1094

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jkoritzinsky
Copy link
Member

@jkoritzinsky jkoritzinsky commented May 3, 2023

Add a .NET 6.0 test target that references Roslyn 4.4 so we have a test suite that references Roslyn 4.X.

Introduce a new API to enable developers to define post-initial-run solution transforms to test incrementality.

Add a Humanizer reference at the same version as Roslyn's dependency to improve the error message wording of the new validation section (can be removed if we so desire).

Note

Notes from @sharwell on specific tests we believe would be valuable. These tests could be provided by the API in this pull request, or through some other means.

  • Ability to verify that a change which did not impact outputs only executed specific incremental steps. Since it's easy (per @jkoritzinsky) to unintentionally break equality comparisons for model objects, there is a likelihood of regression in the incremental evaluation due to equivalent items returning not-equal.
  • Ability to verify that a change which only impacted a subset of outputs only executed specific incremental steps that serve as inputs to other outputs which did not change.
  • Ability to test that a source generator did not include Compilation as part of its incremental state. In other words, after running source generators on a compilation and still holding the GeneratorDriver instance, the Compilation passed to it should be eligible for garbage collection.

@jkoritzinsky jkoritzinsky added the Area-MS.CA.Testing Microsoft.CodeAnalysis.Testing label May 3, 2023
@jkoritzinsky jkoritzinsky requested a review from sharwell May 3, 2023 18:40
@jkoritzinsky jkoritzinsky requested a review from a team as a code owner May 3, 2023 18:40
@@ -70,10 +70,12 @@
<MicrosoftVisualStudioProjectSystemSDKToolsVersion>17.3.195-pre</MicrosoftVisualStudioProjectSystemSDKToolsVersion>
<!-- Libs -->
<DiffPlexVersion>1.5.0</DiffPlexVersion>
<HumanizerCoreVersion>2.14.1</HumanizerCoreVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 I would prefer to remove this dependency before merge, but that's further down the line. I'm mostly focusing on the API itself right now.

@@ -27,5 +28,19 @@ protected override CompilationOptions CreateCompilationOptions()

protected override ParseOptions CreateParseOptions()
=> new CSharpParseOptions(DefaultLanguageVersion, DocumentationMode.Diagnose);

public IncrementalGeneratorExpectedState IncrementalGeneratorState
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 I think we can remove this in favor of changing the type of IncrementalGeneratorStepStates to a new type derived from Dictionary<Type, IncrementalGeneratorExpectedState> that provides a new indexer that creates the value on demand. In practice in means you'd have to type this, but it seems acceptable:

IncrementalGeneratorStepStates =
{
    [typeof(TSourceGenerator)] =
    {
        ExpectedStepStates =
        {
            // ...
        },
    },
},

var secondRunProject = await CreateProjectAsync(new EvaluatedProjectState(incrementalTestState, ReferenceAssemblies), incrementalTestState.AdditionalProjects.Values.Select(additionalProject => new EvaluatedProjectState(additionalProject, ReferenceAssemblies)).ToImmutableArray(), cancellationToken);

driver = driver
.ReplaceAdditionalTexts(secondRunProject.AnalyzerOptions.AdditionalFiles)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ Will the driver recognize cases where a file did not change? For example, consider the case of two additional files. If you change the content of just one of them, will it treat the other one as unchanged?

Comment on lines +34 to +38
if (!TestState.GeneratedSources.Any())
{
// Verify the test state has at least one source, which may or may not be generated
Verify.NotEmpty($"{nameof(TestState)}.{nameof(SolutionState.Sources)}", TestState.Sources);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it okay to have empty sources but non-empty generated sources?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-MS.CA.Testing Microsoft.CodeAnalysis.Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants