New Contributor with some questions

[keith-anders]

Summary

Hello! I want to start contributing to Roslyn. I began by trying to implement csharplang issue 338, which is to allow ref-kind parameters in lambdas with implicit parameter types. I have it working, but I have a small army of questions whose answers I wasn't able to find in the docs. Any help or redirection to appropriate documentation would be appreciated.

Questions

1. How do I reference the issue? I can't find an issue in dotnet/roslyn; should the commit message just have a link to the csharplang issue instead? Do the tests need to refer to the issue? Some existing tests have a WorkItem attribute, and some don't, and it's not obvious to me what the conventions are.
2. Should I pr directly from my fork to dotnet/roslyn/main? Or is there another branch I should target?
3. The docs say to make sure "all Jenkins legs" pass before submitting. Is there a particular sequence of tests that I should run or is the Test.cmd script good enough? Any particular parameters to pass to it? I can try to run them on every combination in the Azure pipelines files, but only to a point. (e.g. I can run them on Debug and Release, but I don't have a ubuntu machine to try to run them on--just Windows 10)
4. What is the appropriate level of unit testing? I tried to thoroughly test everything that I thought needed it, but I don't want to add bloat, especially in a project as massive as roslyn already is. If there are any guidelines, I'd love to read them.
5. Do I need to add a check for feature availability against the language version? I see some of these checks sprinkled throughout the code base, but it's not clear to me whether it's required for every feature and bugfix or just in certain cases.
6. I was a little worried about correctly determining whether I'm parsing a lambda parameter list as opposed to a left deconstruction, but I couldn't think of a line of code that is sufficiently ambiguous for testing. Suggestions or should I not worry about it?
7. My RoslynDeployment and Roslyn.Compilers.Extension F5s in VS do not actually cause the RoslynDev VS instance to use the changed compiler for building, it only picks up the changes for the IDE. I know that the compiler works, however, because if I package it up and install the package in the project with Microsoft.Net.Compilers.Toolset, then it behaves as expected. Any idea what I might have done wrong here?

Another Issue

I'm having some intermittently failing tests, but I don't think it's my code since they also intermittently fail for me on a fresh checkout of dotnet/roslyn/main. If anyone has any idea what is going on, I'd love to know. Otherwise, I'd settle for some sort of indication that it's ok to submit my pr despite these tests. Here is the data I have gathered about these failures:

• All tests ALWAYS pass when run from VS.
• All tests ALWAYS pass if run one test class at a time.
• Failed tests are ALWAYS in the Microsoft.CodeAnalysis.CSharp.EditorFeatures.UnitTests project.
• The number of failing tests varies wildly and randomly from build to build, sometimes as low as 0 and sometimes as high as the mid 100's. It changes randomly even if no rebuild is performed between test runs.
• Failed tests are ALWAYS in partition 10 of their assembly.
• Failed tests are ALWAYS asserting a code fix. Each such test may fail in one of two ways: either no codefix action is offered when one was expected, or the converted code is exactly as expected EXCEPT for primitive types getting converted to their fully qualified names (e.g. converting int to System.Int32) for no reason at all.
• Deleting the artifacts folder and rebuilding before testing again does not seem to make the behavior any different, nor any more or less random.

Conclusion

Pinging @CyrusNajmabadi since he championed this issue.

As mentioned, this is my first contribution to roslyn, so I'm very open to feedback of any type, whether or not it addresses any of my explicit questions. The things I most need to know are probably the things I don't even know to ask about! You can review my changes so far here.

[jasonmalinowski]

To cover a few points here, feel free to submit a draft PR using GitHub's draft PR feature as a way to run all CI jobs, and you can start by submitting that to main. Even within our team we'll often submit draft PRs if we're unsure if our own changes are breaking something since that's easier than trying to run everything locally, so there's no shame in submitting a draft PR with broken tests.

[jasonmalinowski]

And for the ones that are failing when running locally, uh, let us know which tests and if we can repro we'll need to get that fixed.

[jmarolf]

How do I reference the issue? I can't find an issue in dotnet/roslyn; should the commit message just have a link to the csharplang issue instead? Do the tests need to refer to the issue? Some existing tests have a WorkItem attribute, and some don't, and it's not obvious to me what the conventions are.

I would refer you to @alrz's PR that adds a language feature: #52139. It references the proposal and spec from dotnet/csharplang as well as a github issue on dotnet/roslyn outlining the test plan. In general, you do not need to link to github issues in tests unless you are fixing a bug and we may want more context on what the original scenario was.

[jmarolf]

What is the appropriate level of unit testing? I tried to thoroughly test everything that I thought needed it, but I don't want to add bloat, especially in a project as massive as roslyn already is. If there are any guidelines, I'd love to read them.

We do not typically do any unit testing in roslyn. all of our tests are at a feature level. I would file a bug on dotnet/roslyn laying out your test plan for review

[jmarolf]

Do I need to add a check for feature availability against the language version? I see some of these checks sprinkled throughout the code base, but it's not clear to me whether it's required for every feature and bugfix or just in certain cases.

Yes, new language features should only be available in newer language versions. If I target C# 7 I should not get your feature

[jmarolf]

I was a little worried about correctly determining whether I'm parsing a lambda parameter list as opposed to a left deconstruction, but I couldn't think of a line of code that is sufficiently ambiguous for testing. Suggestions or should I not worry about it?

Please lay out these concerns in a test plan

[jmarolf]

My RoslynDeployment and Roslyn.Compilers.Extension F5s in VS do not actually cause the RoslynDev VS instance to use the changed compiler for building, it only picks up the changes for the IDE. I know that the compiler works, however, because if I package it up and install the package in the project with Microsoft.Net.Compilers.Toolset, then it behaves as expected. Any idea what I might have done wrong here?

I'll have a look, this gets broken from time-to-time but it is expected that F5 should work for testing compiler changes. @dotnet/roslyn-compiler how do you validate your changes in Visual Studio today?

[jmarolf]

I'm having some intermittently failing tests,

Please file a bug describing what you are seeing. We take flakey tests seriously :)

[333fred]

I will also say that, for language implementations, please let us know ahead of time that you are interested in making changes. Currently, we're at our limit with the number of community language contributions in progress, and we don't have time to review another contribution at the moment. @jaredpar might be able to comment as to when we'd be able to look at something like this, but in particular it will conflict with the other ongoing lambda changes that are being done for C# 10.