Add Roslyn source generator #200
Conversation
|
My current focus is to get all test scenarios in the System tests to pass for MSTest, but I'm not able to decipher the test-failure for scenario. The output is both massive and cryptic, and I would appreciate help translating the failure into something actionable. But I'm also slightly thwarted by the build here failing for the Analyser package as it's not your standard NuGet package. |
|
Do you mean the This one fails, because the logic in Roslyn Source Generator is missing a special case for ignored examples. In the old generator the special logic is in MsTestV2GeneratorProvider: // MsTest doesn't support to ignore a specific test case / DataRow
if (isIgnored)
{
return;
}If I add a check for the ignore tag to the var moreData = values.Skip(1).ToList();
if (set.Tags.Any(x => x.Equals("ignore", StringComparison.InvariantCultureIgnoreCase)))
continue;
moreData.Add(set.Tags);Hope that helps. 🙂 |
|
Ignored examples don't get executed, so I dropped in a little config switch to enable emitting / omitting code for these examples. I made the default to omit examples with an ignore tag and all the MSTest examples pass now. Next up: fixing the NuGet packaging failure in the build pipeline. |
|
The error NU5017 is quite specific, relating to a lack of assemblies or assembly references in a package: https://learn.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu5017 This is true for Roslyn packages and I don't see this error building locally, even when running an identical |
|
The CI sets two environment variables to true, if you do the same you can reproduce it locally: Likely the second one is causing the problem, as the following is configured in Directory.Build.props: <PropertyGroup Condition="'$(GITHUB_ACTIONS)' == 'true'">
<GitBranch Condition=" '$(GitBranch)' == ''">$(GITHUB_REF_NAME)</GitBranch>
<GitCommitSha Condition=" '$(GitCommitSha)' == ''">$(GITHUB_SHA)</GitCommitSha>
<ContinuousIntegrationBuild>true</ContinuousIntegrationBuild>
<EmbedUntrackedSources>true</EmbedUntrackedSources>
<IncludeSymbols>true</IncludeSymbols>
<SymbolPackageFormat>snupkg</SymbolPackageFormat>
</PropertyGroup>This also creates a A quick fix would be to disable the inclusion of the pdb (and also the snupkg) in the FeatureTestGenerator.csproj: <IncludeSymbols>false</IncludeSymbols> |
|
@gasparnagy We're finally here: a generator that produces C# code for the three supported test frameworks. Would you give me your impression of the code-changes and thoughts on what's next? I think we could certainly do with more testing, but it's also tempting to put it in the hands of users to get real feedback. |
|
I did an initial review, but I provide my notes as a comment, not as a regular review, because it will be easier to track it this way. Most of my comments are not related to a particular code anyway. I numbered the notes, so that we can better track them. General feedbackAfter reviewing the code, I am convinced that the idea and the approach is good and this will be a cool improvement in Reqnroll. So 🙏! The code in general is clean and was understandable for me. Quite a big change, so extra kudos for taking the time. Rollout
Performance reviewI did some performance measures with a project that has 200 scenarios (in 20 files) and with a project that has 5000 scenarios (in 500 files). The performance of the roslyn gen is pretty good, provides a 30-50% improvement over the old one. Building a Reqnroll project with 5000 scenarios took ~10 seconds on my 10 year old dev machine, which is pretty good. Here are the full numbers if you are interested: https://docs.google.com/spreadsheets/d/1dvvKnG6EYdvOgsoVnyMN_6Q5jwkNAmE-/edit?usp=sharing&ouid=113727882162066279604&rtpof=true&sd=true There are some interesting learnings though:
Behavior feedbackAs the code was pretty clean, most of my feedback is rather about the behavior and not the code itself. I have found a few typos in the code, but I commit a fix for them. But once the behavior is finalized I will do another round of code review. For the feedback I did a lot of experimentation with MsTest, but did not try NUnit and xUnit in detail. So it might be that some of my comments are generic, some are MsTest specific. Some of my comments are marked with "TODO" prefix. These are not bugs, but just things that are not yet implemented with the new gen. I thought I will list these, to have a comprehensive task list. You will see that many of the comments are about strange cases that normally never happen. So they seem to be nitpicking, but in this scale they do happen, so we have to ensure that the gen works for them. The comments are in a kind of random order, I could not really group them better.
|
|
For point 10, can't you do this by also generating #line pragmas with column ranges? |
|
Regarding 32: it's because there's a discrepancy between our current implementations: MSTest and xUnit emit these examples, where the nUnit handling omits them. They're excluded early in the pipeline, too late for the framework handler to get involved. The easiest solution was to make it a toggle and have the NUnit package switch it off. |
|
Regarding 3: configuration using an additional file can be done. We'd have to include it in the compilation to have Roslyn be able to access its contents. The major downside of this would be that you'd not be able to influence generation settings on a per file/folder basis as you can with .editorconfig. My personal preference is to migrate users to . editorconfig settings, following the convention of all Roslyn components. It's a new generator with new rules! |
|
Regarding 9: Those directives are to stop the analyzer being included in the compiled output of the project. Essentially, you only want it as a design/build-time dependency and not as a runtime component. It's a standard practice for Roslyn components to be included with these restrictions to avoid them transitively finding their way into projects they aren't wanted in. If you want it, you have to have a direct reference to a package that contains it. |
|
Regarding 8: the nullable stuff is only really useful to aid code that is being written as an extension of the class. I noticed that our classes are marked as partial, so I assume somebody will be referencing them somewhere and would benefit from having the nullable information properly presented. It additionally helps me (and everybody else who has to inspect the generated code) to detect where we maybe aren't handling nullable references properly. Ideally we'd not need to complicate the code with the feature toggle, however we are still supporting much older C# versions without nullable reference support. |
|
Regarding 13: This was a bug in equality checks involving type hierarchies, a particularly prickly topic I have yet to find an airtight pattern for. With the equality checks fixed, the test methods are updated as you type. |
|
Regarding #31: I'm bumping up against the limitations of the Gherkin parser, most notably a lack of precise token positions used for source-mapping. It would be really good to have an implementation that used the Roslyn data structures, so I wouldn't have to keep marshalling data between structures (sometimes introducing off-by-one bugs). Dependencies are a pain-point, having to be embedded in the NuGet package directly and loaded as an analyzer (even when they're not). The model really wants an all-in-one assembly. |
Includes generation of namespaces based on folder structure
|
@Code-Grump I added a list of review comments in the PR description and based on your commits and comments I tried to mark the ones that are handled now. Please review and if any other is also fixed, mark it. Here are my reponses to the other notes:
Ok. I see. These differences are not intentional, but we emited/excluded examples depending on the capabilities of the runner. I don't remember the details, but I can check. So you can have this as an internal parameter (e.g. a bool prop on the test fw implementation classes), but I would not expose this as a configuration parameter for the end user. Let's keep it as an internal decision whether we want or do not emit the ignored examples.
I was thinking this over once more. I understand your points, but I would vote against the
I see. The classes were generated as partial as a kind of back door for special temporary hacks. I don't think anyone uses the partial behavior of the classes. My fear is that the different test framework versions are annotated differently for nullability, so making sure that there are no warnings is an endless fight that has to be redone every time a new test fw version or .NET version comes out. Also when we make a change ourselves but we might forget to verify some combinations when this causes a warning. Many projects are configured in a way that warnings are treated as errors, so if we make a mistake here (in some combination a warning is produced), the users will have no other option than generally ignore the nullable warning for their Reqnroll project, so their own real code will not be checked. So my feeling is that the risk of the harm for an emitted warning is more severe then the benefit we get from the nullable checks in the generated code. But we can make a test period and see if we can get rid of this warnings for all combinations.
Could you please elaborate on this a bit more in detail?
The Gherkin parser is designed in a way that you can provide your own AST builder that builds the structure you want/need. So it would be possible to use the Gherkin parser that is configured with a special AST builder that builds the structures you need immediately during parsing. The AST builders should contain no logic, so by replacing the built-in AST builder with your own one will not change the parsing behavior. (Currently we might do a few extra validations in the AST builder that you would need to duplicate though.)
I understand this, but then we would need to find a way to apply those few additional validation that I mentioned in the review note in your gen as well. As there are only 4 of them, duplicating the code is also acceptable for me. I just don't want to lose them. |
🤔 What's changed?
Adds new Roslyn-based source generator to convert feature files into executable tests.
⚡️ What's your motivation?
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
📋 Checklist:
Tracking review notes:
*.feature.csfiles<PrivateAssets>and<IncludeAssets>ReqnrollFeatureFiles) when Roslyn gen is activeemitIgnoredExamplesconfigThis text was originally taken from the template of the Cucumber project, then edited by hand. You can modify the template here.