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 Emit2 test project #57238

Merged
merged 5 commits into from
Oct 20, 2021
Merged

Add Emit2 test project #57238

merged 5 commits into from
Oct 20, 2021

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Oct 19, 2021

Closes #57226

See https://github.com/dotnet/roslyn/runs/3944469911 for proof that the tests run in CI :)

cc @dotnet/roslyn-compiler

@RikkiGibson RikkiGibson marked this pull request as ready for review October 19, 2021 21:35
@RikkiGibson RikkiGibson requested a review from a team as a code owner October 19, 2021 21:35
@jaredpar
Copy link
Member

Can we link to the issue which inspired this and has the reasons for why we need yet another Emit test project here?

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Please look through our scripts and add this test where appropriate (such as https://github.com/dotnet/roslyn/blob/main/eng/build.ps1#L333)

@RikkiGibson RikkiGibson requested review from a team as code owners October 19, 2021 22:00
@RikkiGibson
Copy link
Contributor Author

Did a search for CSharp\.Emit\.UnitTests and CSharp.Emit.UnitTests, adding parallel entries for Emit2. Any further suggestions on what to search for/update?

@333fred
Copy link
Member

333fred commented Oct 19, 2021

Is there anything special we need to do to mark the test project as non-shipping? Or is the whole test folder automatically marked as such?

@RikkiGibson
Copy link
Contributor Author

RikkiGibson commented Oct 19, 2021

Is there anything special we need to do to mark the test project as non-shipping? Or is the whole test folder automatically marked as such?

It appears that some Arcade configuration sees that the project name ends with UnitTests and sets the 'IsUnitTestProject' property which sets the 'IsShipping=false' property.

https://github.com/dotnet/arcade/blob/5a11ca12dac8353d469045ee794dc54f0ddd49a2/src/Microsoft.DotNet.Arcade.Sdk/tools/Tests.props#L15

https://github.com/dotnet/arcade/blob/main/Documentation/ArcadeSdk.md#isshipping-isshippingassembly-isshippingpackage-isshippingvsix-bool

I verified in the binlog that 'IsShipping' is false for the new project.

@@ -337,6 +337,7 @@ function GetCompilerTestAssembliesIncludePaths() {
$assemblies += " --include '^Microsoft\.CodeAnalysis\.CSharp\.Symbol\.UnitTests$'"
$assemblies += " --include '^Microsoft\.CodeAnalysis\.CSharp\.Semantic\.UnitTests$'"
$assemblies += " --include '^Microsoft\.CodeAnalysis\.CSharp\.Emit\.UnitTests$'"
$assemblies += " --include '^Microsoft\.CodeAnalysis\.CSharp\.Emit2\.UnitTests$'"
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, what does this list do (if you know)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Its the list of tests that are considered 'compiler' so you can run just the compiler tests without everything else.

Copy link
Member

Choose a reason for hiding this comment

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

Which then controls what the IOperation hook is run on.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 5)

@RikkiGibson RikkiGibson merged commit 00a6ed5 into dotnet:main Oct 20, 2021
@ghost ghost added this to the Next milestone Oct 20, 2021
@RikkiGibson RikkiGibson deleted the add-test-project branch October 20, 2021 17:46
@RikkiGibson RikkiGibson modified the milestones: Next, 17.1.P1 Oct 25, 2021
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.

Emit tests are approaching the string limit
5 participants