Add Rebuild.UnitTests #51778

RikkiGibson · 2021-03-10T17:52:40Z

Adds Rebuild.UnitTests project
Adds Platform round trip tests. ~~Currently cases X86 and AnyCpu32BitPreferred are failing.~~ (edit: now are all passing.)

I can't quite figure out how to implement GetPlatform to make all these values round trip. In particular struggling to figure out how to distinguish the AnyCpu case from the X86 case. It seems like when Platform.AnyCpu is provided as an input, we translate it to Machine.Unknown:

roslyn/src/Compilers/Core/Portable/Compilation/Compilation.cs

Lines 1971 to 1973 in b6f8504

 case Platform.AnyCpu: 

 case Platform.AnyCpu32BitPreferred: 

 machine = Machine.Unknown;

However, when I try to read the Machine value back out of the PE later on, it ends up being Machine.I386... I am really not sure what I'm missing or where the value is getting changed.

@jaredpar for review.

RikkiGibson · 2021-03-10T20:47:44Z

After chatting with @jaredpar decided to just add the platform to the PDB compilation options. Let me know if you have objections @tmat or ideas on how to more properly implement this.

Compilers.sln

src/Compilers/Core/Portable/Compilation/Platform.cs

src/Compilers/Core/Rebuild/BuildConstructor.cs

src/Compilers/Core/RebuildTest/CSharpRebuildTests.cs

jaredpar · 2021-03-10T21:04:41Z

src/Compilers/Core/RebuildTest/CSharpRebuildTests.cs

+ var (compilation, isError) = bc.CreateCompilation(optionsReader, "test", sources, references);
+
+ Assert.False(isError);
+ Assert.Equal(platform, compilation!.Options.Platform);


Think we need to take the next step and ensuring the original and rebuilt compilation have the same byte[] output. Consider doing as a helper so future tests can just build on top of that.

I think this is a good idea, but there are issues with it to iron out that I'm not sure I want to block on.

Fine on working it out later but let's file an issue to track.

jaredpar · 2021-03-10T21:05:20Z

src/Compilers/Core/RebuildTest/CSharpRebuildTests.cs

+namespace Rebuild.UnitTests
+{
+ //[CompilerTrait(CompilerFeature.)]
+ public class CSharpRebuildTests : CSharpTestBase


What about VB tests here?

I think it would be good to have a VB version of this test. I guess in that case we'd want to add a reference to the VB test helpers?

My instinct was to have an abstract base class and then have both a VB and C# derivation. Hoping that it would be easy to abstract over the languages this way.

I'd like to introduce a few more test cases and then try to figure out what test helpers would be most useful.

src/Compilers/Core/Rebuild/BuildConstructor.cs

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

RikkiGibson · 2021-03-11T19:52:25Z

@jaredpar have responded to the feedback.

cston · 2021-03-11T20:11:55Z

src/Compilers/Core/Portable/Compilation/Platform.cs

@@ -2,6 +2,8 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

+using Roslyn.Utilities;


Roslyn.Utilities [](start = 6, length = 16)

Is this needed?

No, thanks, have removed.

src/Compilers/Core/Portable/Compilation/Platform.cs

cston · 2021-03-11T20:42:34Z

src/Compilers/Core/Rebuild/BuildConstructor.cs

@@ -176,6 +177,11 @@ private static (OptimizationLevel, bool) GetOptimizationLevel(string? optimizati
 _ => throw new InvalidDataException($"Optimization \"{optimizationLevel}\" level not recognized")
 };

+ private static Platform GetPlatform(string? platform)
+ => platform is null


=> [](start = 8, length = 3)

Should this be indented? #Closed

cston · 2021-03-11T20:46:03Z

src/Compilers/Core/RebuildTest/Microsoft.CodeAnalysis.Rebuild.UnitTests.csproj

+ <ProjectReference Include="..\Rebuild\Rebuild.csproj" />
+ </ItemGroup>
+ <ItemGroup>
+ <PackageReference Include="Moq" Version="$(MoqVersion)" />


[](start = 4, length = 58)

Is this package used? #Closed

cston · 2021-03-11T21:03:58Z

src/Compilers/Core/RebuildTest/OptionRoundTripTests.cs

+
+ var factory = LoggerFactory.Create(configure => { });
+ var logger = factory.CreateLogger(path);
+ // TODO: shouldn't need to pass a logger.


If we need an ILogger instance, could we create a simple implementation of ILogger in this test class?

Yes, but it feels like users of the public API shouldn't be required to provide a logger, and shouldn't provide it as its own parameter. Maybe a property on an options object that can be specified if the user wants, but not a parameter to public API that they have to deal with.

cston · 2021-03-11T21:10:27Z

src/Compilers/Core/RebuildTest/OptionRoundTripTests.cs

+ {
+ var text = st.GetText();
+ return new ResolvedSource(OnDiskPath: null, text, new SourceFileInfo(path, text.ChecksumAlgorithm, text.GetChecksum().ToArray(), text, embeddedCompressedHash: null));
+ }).ToImmutableArray();


ToImmutableArray [](start = 15, length = 16)

Could use SyntaxTrees.SelectAsArray(...) instead.

I think this project is missing an IVT to MS.CA for this. I will punt on it for now if that's ok.

…nto rebuild-unittests

tmat · 2021-03-12T04:20:58Z

src/Compilers/Core/Portable/PEWriter/MetadataWriter.PortablePdb.cs

@@ -888,6 +888,9 @@ private void EmbedCompilationOptions(BlobReader? pdbCompilationOptionsReader, Co
 WriteValue(CompilationOptionNames.Optimization, optimizationLevel.ToPdbSerializedString(debugPlusMode));
 }

+ var platform = module.CommonCompilation.Options.Platform;
+ WriteValue(CompilationOptionNames.Platform, platform.ToString());


Platform can't be determined from the CLR/PE header flags of the dll? Seems like it should be possible.

It may be possible, but I wasn't able to get all the values to round trip. The CoffHeader.Machine, for example, is not enough information. There must be other bits that imply Platform.AnyCpu and Platform.AnyCpu32BitPreferred that I just wasn't able to find.

These are stored in CorHeader flags:
https://source.dot.net/#System.Reflection.Metadata/System/Reflection/PortableExecutable/CorHeader.cs,1f6cfc9c2185aaf4,references

Filed #51941 to make sure we get to this. Thanks

RikkiGibson added 2 commits March 9, 2021 17:17

Add rebuild test project

9d83e11

Add platform round trip test

5341001

RikkiGibson requested review from a team as code owners March 10, 2021 17:52

dotnet-issue-labeler bot added the Area-Compilers label Mar 10, 2021

Add Platform to PDB compilation options

a3187d7

jaredpar requested changes Mar 10, 2021

View reviewed changes

RikkiGibson added 3 commits March 10, 2021 13:21

Rename unit test project. Remove dead comment

e29d5b2

fix IVTs. Use Enum.GetValues.

cc0e63e

Use Enum.ToString

2cb6884

jaredpar reviewed Mar 11, 2021

View reviewed changes

src/Compilers/Core/Rebuild/BuildConstructor.cs Outdated Show resolved Hide resolved

RikkiGibson and others added 4 commits March 11, 2021 11:31

Update src/Compilers/Core/Rebuild/BuildConstructor.cs

e828978

Co-authored-by: Jared Parsons <jaredpparsons@gmail.com>

Address some feedback

09f238a

rename file

b83579f

Convert more option names

5fa70d1

RikkiGibson requested a review from jaredpar March 11, 2021 19:52

jaredpar approved these changes Mar 11, 2021

View reviewed changes

cston reviewed Mar 11, 2021

View reviewed changes

RikkiGibson commented Mar 11, 2021

View reviewed changes

src/Compilers/Core/Portable/Compilation/Platform.cs Outdated Show resolved Hide resolved

Update src/Compilers/Core/Portable/Compilation/Platform.cs

9b047b2

cston reviewed Mar 11, 2021

View reviewed changes

RikkiGibson added 4 commits March 11, 2021 16:00

Update Microsoft.CodeAnalysis.Rebuild.UnitTests.csproj

c6533e8

Fix indent

57047c3

Merge branch 'main' of github.com:dotnet/roslyn into rebuild-unittests

c51ec95

Add EditorFeatures2.UnitTests.dll to exclusions

42b3d6e

RikkiGibson added 2 commits March 11, 2021 18:16

Exclude new test project from rebuild

aa78ba5

Merge branch 'dev/rigibson/fix-rebuild' of github.com:dotnet/roslyn i…

1a7d1a5

…nto rebuild-unittests

cston approved these changes Mar 12, 2021

View reviewed changes

RikkiGibson merged commit 062355a into dotnet:main Mar 12, 2021

RikkiGibson deleted the rebuild-unittests branch March 12, 2021 03:53

RikkiGibson mentioned this pull request Mar 12, 2021

Add EditorFeatures2.UnitTests.dll to exclusions #51824

Closed

tmat reviewed Mar 12, 2021

View reviewed changes

allisonchou added this to the Next milestone Mar 12, 2021

RikkiGibson mentioned this pull request Mar 17, 2021

Remove Platform entry from PDB compilation options #51941

Open

allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Rebuild.UnitTests #51778

Add Rebuild.UnitTests #51778

RikkiGibson commented Mar 10, 2021 •

edited

Loading

RikkiGibson commented Mar 10, 2021

jaredpar Mar 10, 2021

RikkiGibson Mar 10, 2021

jaredpar Mar 11, 2021

jaredpar Mar 10, 2021

RikkiGibson Mar 10, 2021

jaredpar Mar 11, 2021

RikkiGibson Mar 11, 2021

RikkiGibson commented Mar 11, 2021

cston Mar 11, 2021

RikkiGibson Mar 11, 2021

cston Mar 11, 2021 •

edited

Loading

RikkiGibson Mar 12, 2021

cston Mar 11, 2021 •

edited

Loading

RikkiGibson Mar 12, 2021

cston Mar 11, 2021 •

edited

Loading

RikkiGibson Mar 11, 2021

cston Mar 11, 2021

RikkiGibson Mar 12, 2021

tmat Mar 12, 2021 •

edited

Loading

RikkiGibson Mar 12, 2021

tmat Mar 12, 2021

RikkiGibson Mar 17, 2021

	case Platform.AnyCpu:
	case Platform.AnyCpu32BitPreferred:
	machine = Machine.Unknown;

Add Rebuild.UnitTests #51778

Add Rebuild.UnitTests #51778

Conversation

RikkiGibson commented Mar 10, 2021 • edited Loading

RikkiGibson commented Mar 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Mar 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Mar 10, 2021 •

edited

Loading

cston Mar 11, 2021 •

edited

Loading

cston Mar 11, 2021 •

edited

Loading

cston Mar 11, 2021 •

edited

Loading

tmat Mar 12, 2021 •

edited

Loading