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

Move baselines out of source generator tests #8823

Closed
wants to merge 2 commits into from

Conversation

jjonescz
Copy link
Member

Follow up on #8242 (comment).

@jjonescz jjonescz added the area-compiler Umbrella for all compiler issues label Jun 12, 2023
@jjonescz jjonescz added this to the 17.7 P3 milestone Jun 12, 2023
@jjonescz jjonescz marked this pull request as ready for review June 12, 2023 13:36
@jjonescz jjonescz requested a review from a team as a code owner June 12, 2023 13:36
@333fred
Copy link
Member

333fred commented Jun 12, 2023

So, I honestly, truly prefer the test output inline with the test, as I find it makes reviewing significantly easier. Am I the only one? I'm willing to concede if so, but if not, I'd much rather keep the current structure, even if it's inconsistent with some of the other test projects. @dotnet/razor-compiler for opinions.

@jjonescz
Copy link
Member Author

it makes reviewing significantly easier

I can see that. But there are also disadvantages:

  • the inline baselines cannot be automatically updated (unless we write a tooling for that); that means lots of manual copy-pasting (Razor codegen updates tend to change lots of baselines - albeit we can mitigate that by having less baselines in tests in general),
  • the test files can grow large which makes them harder to navigate around.

@jjonescz jjonescz removed this from the 17.7 P3 milestone Jun 30, 2023
@jaredpar jaredpar added this to the 17.8 Planning milestone Jul 12, 2023
@chsienki
Copy link
Contributor

IndecisiveIDontKnowGIF

So I was the one who originally put these in code. Like @333fred I prefer having it in the test so you can easily see the behaviour inline with the test.

However, I'm extremely sympathetic to the point that it makes the tests unwieldy, and it would definitely be nice to be able to update the baselines rather than having to do it manually.

@jjonescz
Copy link
Member Author

However, I'm extremely sympathetic to the point that it makes the tests unwieldy, and it would definitely be nice to be able to update the baselines rather than having to do it manually.

Perhaps we can do that even if they are in source, using Roslyn, or something like https://github.com/jjonescz/roslyn-test-updater. Closing this for now.

@jjonescz jjonescz closed this Jul 14, 2023
@jjonescz jjonescz deleted the sg-test-baselines branch July 14, 2023 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-compiler Umbrella for all compiler issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants