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

Verify conversion kinds in native int conversion tests #60895

Merged
merged 4 commits into from
Apr 22, 2022

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Apr 21, 2022

Test plan #60578

I recommend github.dev for presenting a minimal diff: https://github.dev/dotnet/roslyn/pull/60895

@jcouv jcouv requested a review from a team as a code owner April 21, 2022 23:55
@jcouv jcouv self-assigned this Apr 21, 2022
@cston
Copy link
Member

cston commented Apr 22, 2022

        private ConversionKind[] kinds;

readonly


In reply to: 1105964941


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:7290 in 5a2ad5e. [](commit_id = 5a2ad5e, deletion_comment = False)

@cston
Copy link
Member

cston commented Apr 22, 2022

        public static ExpectedConversion Identity = ConversionKind.Identity;

readonly for each of the static fields.


In reply to: 1105965012


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:7292 in 5a2ad5e. [](commit_id = 5a2ad5e, deletion_comment = False)

@cston
Copy link
Member

cston commented Apr 22, 2022

        public void AssertMatches(Conversion conversion)

Consider calling this bool Equals(Conversion) and let the caller assert, so that this helper class is independent of the test framework.


In reply to: 1105968021


In reply to: 1105968021


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:7327 in 5a2ad5e. [](commit_id = 5a2ad5e, deletion_comment = False)

@jcouv
Copy link
Member Author

jcouv commented Apr 22, 2022

        public void AssertMatches(Conversion conversion)

It's actually better to assert here, because the Assert.Equal on enumerables prints out the actual list. Since I'm going to re-use this helper for new tests, that'll be helpful.


In reply to: 1105968021


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:7327 in 5a2ad5e. [](commit_id = 5a2ad5e, deletion_comment = False)

@cston
Copy link
Member

cston commented Apr 22, 2022

    internal class ExpectedConversion

Could we use Conversion (or ConversionKind) directly rather than introducing a new type?


In reply to: 1105977264


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:7288 in 5a2ad5e. [](commit_id = 5a2ad5e, deletion_comment = False)

@jcouv
Copy link
Member Author

jcouv commented Apr 22, 2022

    internal class ExpectedConversion

I don't mind. Switched to ConversionKind[]


In reply to: 1105977264


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:7288 in 5a2ad5e. [](commit_id = 5a2ad5e, deletion_comment = False)

@cston
Copy link
Member

cston commented Apr 22, 2022

using System.Diagnostics;

Is this used?


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs:21 in f26191f. [](commit_id = f26191f, deletion_comment = False)

@jcouv jcouv enabled auto-merge (squash) April 22, 2022 04:30
@jcouv jcouv added the Test Test failures in roslyn-CI label Apr 22, 2022
@jcouv jcouv merged commit fa94573 into dotnet:features/numeric-intptr Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Test Test failures in roslyn-CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants