Skip to content

Add Default Interface Implementation tests for VB#35957

Merged
AlekseyTs merged 3 commits intodotnet:masterfrom
AlekseyTs:VBDIM
Jun 2, 2019
Merged

Add Default Interface Implementation tests for VB#35957
AlekseyTs merged 3 commits intodotnet:masterfrom
AlekseyTs:VBDIM

Conversation

@AlekseyTs
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs commented May 24, 2019

@AlekseyTs AlekseyTs requested a review from a team as a code owner May 24, 2019 22:56
@AlekseyTs AlekseyTs added the Test Test failures in roslyn-CI label May 24, 2019
@AlekseyTs
Copy link
Copy Markdown
Contributor Author

AlekseyTs commented May 24, 2019

@dotnet/roslyn-compiler Please review

@AlekseyTs
Copy link
Copy Markdown
Contributor Author

AlekseyTs commented May 28, 2019

@dotnet/roslyn-compiler Please review, this is a test only change

jcouv
jcouv reviewed May 28, 2019
View reviewed changes
...Compilers/VisualBasic/Test/Symbol/Microsoft.CodeAnalysis.VisualBasic.Symbol.UnitTests.vbproj
<PropertyGroup>
<OutputType>Library</OutputType>
<TargetFrameworks>$(RoslynPortableTargetFrameworks)</TargetFrameworks>
<TargetFrameworks>net472;netcoreapp3.0</TargetFrameworks>
Copy link
Copy Markdown
Member

@jcouv jcouv May 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

net472;netcoreapp3.0 [](start = 22, length = 20)

What values would $(RoslynPortableTargetFrameworks) produce? (ie. what's the difference?) #Closed

Copy link
Copy Markdown
Contributor Author

@AlekseyTs AlekseyTs May 28, 2019

Choose a reason for hiding this comment

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

What values would $(RoslynPortableTargetFrameworks) produce? (ie. what's the difference?)

I think it is: "net472;netcoreapp2.1"

In reply to: 288298770 [](ancestors = 288298770)

jcouv
jcouv reviewed May 28, 2019
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs
}
}

[Fact]
Copy link
Copy Markdown
Member

@jcouv jcouv May 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

[Fact] [](start = 7, length = 7)

Not related to this PR: have we thought about the use of Obsolete attribute in DIM? Consider adding some tests if interesting. #Closed

Copy link
Copy Markdown
Contributor Author

@AlekseyTs AlekseyTs May 28, 2019

Choose a reason for hiding this comment

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

Not related to this PR: have we thought about the use of Obsolete attribute in DIM? Consider adding some tests if interesting.

Unless you have something specific in mind, I do not believe there is specific interaction between DIM and Obsolete attribute.

In reply to: 288299204 [](ancestors = 288299204)

@jcouv
Copy link
Copy Markdown
Member

jcouv commented May 28, 2019 •
edited by AlekseyTs
Loading 

    Private Function GetCSharpCompiation(csSource As String, Optional additionalReferences As MetadataReference() = Nothing, Optional targetFramework As TargetFramework = TargetFramework.NetStandardLatest) As CSharp.CSharpCompilation

typo: Compilation #Resolved

Refers to: src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/DefaultInterfaceImplementationTests.vb:18 in 24234ef. [](commit_id = 24234ef, deletion_comment = False)

@jcouv
Copy link
Copy Markdown
Member

jcouv commented May 28, 2019 •
edited by AlekseyTs
Loading 

        Dim csCompiation = GetCSharpCompiation(csSource).EmitToImageReference()

typo: csCompilation (also in other tests below) #Resolved

Refers to: src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/DefaultInterfaceImplementationTests.vb:43 in 24234ef. [](commit_id = 24234ef, deletion_comment = False)

@jcouv
Copy link
Copy Markdown
Member

jcouv commented May 28, 2019 •
edited
Loading 

        CompileAndVerify(comp1, expectedOutput:=If(ExecutionConditionUtil.IsMonoOrCoreClr, "C.M1", Nothing), verify:=VerifyOnMonoOrCoreClr)

I didn't quite follow this.
The verify parameter controls whether to run PEVerify. If we can't run PEVerify on mono or coreCLR, it feels that should be handled inside our test framework, rather than in individual tests. #Closed

Refers to: src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/DefaultInterfaceImplementationTests.vb:98 in 24234ef. [](commit_id = 24234ef, deletion_comment = False)

@AlekseyTs
Copy link
Copy Markdown
Contributor Author

AlekseyTs commented May 28, 2019 

        CompileAndVerify(comp1, expectedOutput:=If(ExecutionConditionUtil.IsMonoOrCoreClr, "C.M1", Nothing), verify:=VerifyOnMonoOrCoreClr)

If we can't run PEVerify on mono or coreCLR, it feels that should be handled inside our test framework, rather than in individual tests.

It is the opposite, we cannot verify this code when the test is running on Desktop.

In reply to: 496691294 [](ancestors = 496691294)

Refers to: src/Compilers/VisualBasic/Test/Symbol/SymbolsTests/DefaultInterfaceImplementationTests.vb:98 in 24234ef. [](commit_id = 24234ef, deletion_comment = False)

jcouv
jcouv approved these changes May 28, 2019
View reviewed changes
Copy link
Copy Markdown
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 modulo typo. Thanks
Skimmed through the tests, as there's so many.

@AlekseyTs AlekseyTs merged commit a3f2539 into dotnet:master Jun 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcouv jcouv jcouv approved these changes

Assignees

No one assigned

Labels

Area-Compilers Feature - Default Interface Impl Default Interface Implementation Test Test failures in roslyn-CI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@AlekseyTs @jcouv