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

Test base #19521

Merged
merged 7 commits into from
May 17, 2017
Merged

Test base #19521

merged 7 commits into from
May 17, 2017

Conversation

tmat
Copy link
Member

@tmat tmat commented May 15, 2017

Test only change (refactoring).

Currently many test classes across Roslyn compiles and services derive from TestBase. TestBase has become a kitchen sink of helpers. Some of them make sense for all tests, but a lot of them are specific to compilers and are not relevant to services. This change takes a step at separating compiler specific helpers, such as PDB validation to a separate class and project -- Roslyn.Test.PdbUtilities. Long term we could move more IL and metadata validation to this project and rename it to CompilerTestUtilities.

  • Move CompilationVerifier out of TestBase class.
  • Move all PDB validation helpers to PdbValidation class.
  • Move PdbValidation class to Roslyn.Test.PdbUtilities project.

@tmat tmat added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 15, 2017
@tmat tmat added Test Test failures in roslyn-CI and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels May 15, 2017
@tmat
Copy link
Member Author

tmat commented May 15, 2017

@dotnet/roslyn-compiler Please review -- this is test only change. Recommended to review each commit separately.

@tmat
Copy link
Member Author

tmat commented May 15, 2017

@ivanbasov @jinujoseph

@ivanbasov
Copy link
Contributor

:shipit:

@tmat
Copy link
Member Author

tmat commented May 17, 2017

@dotnet/roslyn-compiler Anybody else interested in reviewing?

@jcouv
Copy link
Member

jcouv commented May 17, 2017

Starting review.

private IEnumerable<IModuleSymbol> GetReferencesToModuleSymbols(IEnumerable<MetadataReference> references, MetadataImportOptions importOptions)
{
var dummy = _compilation
.RemoveAllReferences()
Copy link
Member

Choose a reason for hiding this comment

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

RemoveAllReferences is just the same as WithReferences with some empty array. What is the benefit of calling WithReferences twice?


namespace Microsoft.CodeAnalysis.Test.Utilities
{
internal sealed class CompilationDifference
public sealed class CompilationDifference
Copy link
Member

Choose a reason for hiding this comment

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

Why public?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no need for it to be internal. Public is easier as it avoids more internals visible to between test assemblies.

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

@tmat
Copy link
Member Author

tmat commented May 17, 2017

@jcouv Thanks for review.

@tmat tmat merged commit fc4739a into dotnet:master May 17, 2017
@tmat tmat deleted the TestBase branch May 17, 2017 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-already-signed Test Test failures in roslyn-CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants