Skip to content

Moved VB tests to test emitted attributes vs source synthesized ones#25032

Merged
OmarTawfik merged 2 commits intodotnet:masterfrom
OmarTawfik:fix-vb-attr-tests
May 15, 2018
Merged

Moved VB tests to test emitted attributes vs source synthesized ones#25032
OmarTawfik merged 2 commits intodotnet:masterfrom
OmarTawfik:fix-vb-attr-tests

Conversation

@OmarTawfik
Copy link
Copy Markdown
Contributor

@OmarTawfik OmarTawfik commented Feb 24, 2018 •
edited
Loading

Closes #18799

@OmarTawfik OmarTawfik self-assigned this Feb 24, 2018
@OmarTawfik OmarTawfik requested a review from a team as a code owner February 24, 2018 10:04
@OmarTawfik OmarTawfik removed the request for review from a team February 24, 2018 10:07
@OmarTawfik OmarTawfik added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Feb 24, 2018
@OmarTawfik OmarTawfik changed the title Moved VB tests to test emitted attributes vs source synthesized ones [WIP] Moved VB tests to test emitted attributes vs source synthesized ones Feb 24, 2018
@OmarTawfik OmarTawfik requested review from a team as code owners April 9, 2018 22:45
@OmarTawfik OmarTawfik changed the base branch from dev15.7.x to master April 9, 2018 23:43
@OmarTawfik OmarTawfik removed request for a team April 9, 2018 23:43
@OmarTawfik OmarTawfik added Area-Compilers Test Test failures in roslyn-CI labels Apr 9, 2018
@OmarTawfik OmarTawfik changed the title [WIP] Moved VB tests to test emitted attributes vs source synthesized ones Moved VB tests to test emitted attributes vs source synthesized ones Apr 10, 2018
@OmarTawfik OmarTawfik requested a review from a team April 10, 2018 22:14
@OmarTawfik OmarTawfik removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 10, 2018
@OmarTawfik OmarTawfik added this to the 15.8 milestone Apr 10, 2018
@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented Apr 10, 2018 •
edited
Loading

cc @dotnet/roslyn-compiler #Closed

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented Apr 18, 2018 •
edited
Loading

@dotnet/roslyn-compiler any reviewers? this is a test-only change.

#Closed

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented Apr 24, 2018 •
edited
Loading

@dotnet/roslyn-compiler for review. #Closed

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading 

        Return result

Consider adding a helper method which converts all members of an enum to a List(Of Object()) that works for theory. This pattern is used at least twice in this PR. #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:37 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = False)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited
Loading 

    Public Sub Accessors(optimizationLevel As OptimizationLevel)

Glad we're making better use of Theory's i the code. #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:136 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = False)

jaredpar
jaredpar previously approved these changes Apr 24, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

:shipit:

@jaredpar jaredpar dismissed their stale review April 24, 2018 22:33

revoking review

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited
Loading 

            Dim comp = CreateCompilationWithMscorlib40(source, options:=options)

Next time prefer doing the theory conversion as a separate commit. Makse the other changes easier to review. #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:59 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = True)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading 

                                                            attrs.Single(Function(a) a.AttributeClass.Name = "DebuggerBrowsableAttribute").ConstructorArguments(0).Value())

Consider using a helper to extract the constructor value as an enum. That would avoid the new casts and possible remove duplicate code. #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:112 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = False)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading 

        Dim compilationRelaxationsAttrType As NamedTypeSymbol = compilerServicesNS.GetTypeMember("CompilationRelaxationsAttribute")

Before there was an implicit assertion that the attribute was a part of the System.CompilerServices namespace. It seems we lost that in the new code. #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:573 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = True)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading 

        Assert.Equal("RuntimeCompatibilityAttribute", attribute.AttributeClass.Name)

Before there was an implicit assertion of the full name of the attribute. Seems we've lost that here. #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:631 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = False)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading 

            CompilationUtils.AssertNoErrors(compilation)

Is there an equivalent to this assert in the new code? #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:709 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = True)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading 

            CompilationUtils.AssertNoErrors(compilation)

Is there an equivalent to this assert in the new code? #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:754 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = True)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading 

                                     Assert.Equal(3, attributes.Length)

Why did the code move from having 2 attributes to 3? #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:799 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = False)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading

<Module: RuntimeCompatibilityAttribute>

Why was this removed? #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:789 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = True)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading 

            CreateCompilationWithMscorlib40(source, options:=New VisualBasicCompilationOptions(outputKind, optimizationLevel:=OptimizationLevel.Release)),

Shouldn't this be indented one level? #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:923 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = False)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading 

        Assert.Equal("DebuggableAttribute", attribute.AttributeClass.Name)

There used to be an implicit assert about the full name of this type that we've lost in the new code. #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:937 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = False)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading 

    Private Sub TestDebuggableAttributeMatrix(source As String, validator As Action(Of VisualBasicCompilation), Optional includeMscorlibRef As Boolean = True, Optional compileAndVerify As Boolean = True)

Why were these methods removed? #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:941 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = True)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading 

                    Assert.Equal(0, attributes.Length)

Assert.Empty? #Resolved

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:982 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = False)

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented Apr 24, 2018 •
edited by OmarTawfik
Loading

done with review pass 1 #Resolved

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented May 7, 2018 

            CompilationUtils.AssertNoErrors(compilation)

CompileAndVerify() will fail if there are any errors

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

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:709 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = True)

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented May 7, 2018 

            CompilationUtils.AssertNoErrors(compilation)

CompileAndVerify() will fail if there are any errors

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

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:754 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = True)

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented May 7, 2018 

    Private Sub TestDebuggableAttributeMatrix(source As String, validator As Action(Of VisualBasicCompilation), Optional includeMscorlibRef As Boolean = True, Optional compileAndVerify As Boolean = True)

Replaced by the

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

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:941 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = True)

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented May 8, 2018

<Module: RuntimeCompatibilityAttribute>

error BC30549: Attribute 'RuntimeCompatibilityAttribute' cannot be applied to a module.
Check the test: RuntimeCompatibilityAttributeCannotBeAppliedToModule

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

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:789 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = True)

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented May 8, 2018 

                                     Assert.Equal(3, attributes.Length)

Because this is testing the compiled assembly. The compiler also adds Debuggable.

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

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:799 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = False)

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented May 8, 2018 •
edited
Loading 

        Return result

I'd prefer to keep this direct. The pattern is straightforward and short enough?

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

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:37 in 4fc6afa. [](commit_id = 4fc6afa1484a4c9857aecb0044d7861594a69e54, deletion_comment = False)

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented May 8, 2018 •
edited
Loading

@jaredpar comments addressed.
Had to rebase because of the 15.8 project system changes :( so comments might get moved a bit. #Resolved

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented May 8, 2018 

        Return result

At the least I think this could benefit from having an API that returned a typed collection of Enum values from an enum.

In reply to: 387245899 [](ancestors = 387245899,384095345)

Refers to: src/Compilers/VisualBasic/Test/Emit/Attributes/AttributeTests_Synthesized.vb:37 in 6788668. [](commit_id = 6788668, deletion_comment = False)

jaredpar
jaredpar approved these changes May 8, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

:shipit:

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented May 8, 2018

@dotnet-bot test windows_debug_unit64_prtest

@OmarTawfik
Copy link
Copy Markdown
Contributor Author

OmarTawfik commented May 9, 2018

@jaredpar for ask mode approval.

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented May 11, 2018

approved

@jaredpar
Copy link
Copy Markdown
Member

jaredpar commented May 11, 2018 •
edited
Loading

Note: this is test only hence it doesn't need ask mode approval.

@OmarTawfik OmarTawfik merged commit 18cdf8d into dotnet:master May 15, 2018
@OmarTawfik OmarTawfik deleted the fix-vb-attr-tests branch May 15, 2018 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaredpar jaredpar jaredpar approved these changes

Assignees

@OmarTawfik OmarTawfik

Labels

Area-Compilers Test Test failures in roslyn-CI

Projects

None yet

Milestone

15.8

Development

Successfully merging this pull request may close these issues.

2 participants

@OmarTawfik @jaredpar