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 unit tests for C# source generators #82955

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

paulloz
Copy link
Member

@paulloz paulloz commented Oct 7, 2023

Following the discussions in #64899.

We want to be able to notice regressions / breaking changes in the C# source generators. The idea would be to base the tests on the current Godot.SourceGenerators.Sample project, eventually running them on CI when Godot.SourceGenerators is modified. I'm not sure if the sample project would still have a reason to exist once a proper testing pipeline is set up?

Roslyn documentation on the subject can be found here and here.

Following is the current list of generators we'd want to have tests for:

  • ScriptMethodsGenerator
  • ScriptPathAttributeGenerator
  • ScriptPropertiesGenerator
  • ScriptPropertyDefValGenerator
  • ScriptSerializationGenerator
  • ScriptSignalsGenerator

The current draft is a simple proof of concept xUnit project that loads the appropriate assemblies and run a couple of (almost) empty tests against our ScriptPropertiesGenerator.
I still need to decide on a good way to load test data and expected results, and avoid the huge string blocks. The less friction there is on writing tests, the better.

Edit 1:
Blocks of strings are probably fine, at least for now. I roughly pulled everything that was in the sample project into tests (see list above). A couple still need a bit of work to be usable.

Production edit: closes godotengine/godot-roadmap#49

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

I'm not sure if the sample project would still have a reason to exist once a proper testing pipeline is set up?

My idea was to replace the sample project with the tests project, so I'd remove it when we're done.

@raulsntos
Copy link
Member

I still need to decide on a good way to load test data and expected results, and avoid the huge string blocks. The less friction there is on writing tests, the better.

I see what you mean, but I think writing the huge string blocks is probably the easiest way to do it. An alternative would be to parse the output with Roslyn, but I'd say using Roslyn APIs to verify the output is more complicated and error-prone.

@paulloz
Copy link
Member Author

paulloz commented Oct 7, 2023

Yeah, I agree.
I was thinking we may be able to externalize input / expected output content in separate files. And then use [Theory] + [InlineData(inputFile, expectedOutputFile)] to load them.

Edit:
Don't mind me, it wouldn't really be that much more usable. You're right, big blocks of strings are fine, at least for now.

@paulloz paulloz force-pushed the testing-source-generators branch from 7028646 to 6008847 Compare October 7, 2023 14:44
@jolexxa
Copy link
Contributor

jolexxa commented Oct 8, 2023

Wow, this is great 🚀 you already wrote a bunch of tests, too!

Maybe in another PR we could add a "test cases" project. I've found it really useful when building source generators, in addition to a separate unit testing project. The test cases are just little examples of all the things you want the generators to run on and build successfully.

The idea being you add a test case that should build without errors. I add a new one every time someone files a bug, use it to debug and fix the generator, and then leave it in to prevent regressions, etc. It can be great in a pinch, too, when there isn't time or bandwidth for unit tests. You can also write unit tests for the test case examples and ofc the unit test project won't even run if the generator fails to build or builds incorrectly. Testing the test cases also lets you verify the logic of the generated code works as expected, not just that it is present.

@paulloz paulloz force-pushed the testing-source-generators branch from 6008847 to 73942ae Compare October 14, 2023 15:37
@paulloz paulloz marked this pull request as ready for review October 14, 2023 20:32
@paulloz paulloz requested a review from a team as a code owner October 14, 2023 20:32
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Thanks again for doing all this work, and sorry it took me so long to review it.

[Fact]
public async void ScriptBoilerplate()
{
await CSharpSourceGeneratorVerifier<ScriptSerializationGenerator>.VerifyNoCompilerDiagnostics(
Copy link
Member

Choose a reason for hiding this comment

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

I feel a bit hesitant to use CompilerDiagnostics.None. Could this hide errors that we may want to test for? I guess we would also need to run the ScriptPropertiesGenerator to avoid the compiler errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm unsure too. My guess is at this point we're verifying if the generated sources match what we expect them to be. So the expected result should be error free already. But it'd probably be nice to also always run diagnostics in the future.

Copy link
Member

@raulsntos raulsntos Dec 19, 2023

Choose a reason for hiding this comment

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

That makes sense, my thinking was we should verify that the generated code was also valid (i.e.: it compiles) which is something that the old SourceGenerators.Sample would check for because we're building the project in CI so if the generated code was invalid it would fail to build. If we assume the expected TestData is valid and compiles then I guess it's fine, although I feel like it makes the tests a bit less robust.

@raulsntos raulsntos modified the milestones: 4.x, 4.3 Dec 17, 2023
@paulloz paulloz force-pushed the testing-source-generators branch from 04bed2b to bf3caca Compare December 18, 2023 17:38
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! Oh, don't forget to squash the commits.

The next steps should be running the tests in CI and adding tests for the diagnostics we report.

- Bootstrap xUnit project to test source generators
- Implement source generator tests
- Better tests structure (put test data in cs files)
- Enable `ScriptSerializationGeneratorTests`
- Enable `ScriptPathAttributeGeneratorTests`
- Fix `NesterClass` -> `NestedClass`
- Use `Path.Combine` when dealing with paths
- Copy test data to the output directory
@paulloz paulloz force-pushed the testing-source-generators branch from bf3caca to b352bdc Compare December 19, 2023 17:28
@paulloz
Copy link
Member Author

paulloz commented Dec 19, 2023

Looks good to me, thanks! Oh, don't forget to squash the commits.

Done.

The next steps should be running the tests in CI and adding tests for the diagnostics we report.

Probably also make a quick pass to nuke the samples project at some point.

@YuriSizov YuriSizov changed the title Setup unit testing for C# source generators Add unit tests for C# source generators Dec 19, 2023
@YuriSizov YuriSizov merged commit c8d0325 into godotengine:master Dec 19, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants