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

Editing tests for support nullable in ENC #36770

Merged
merged 3 commits into from Jul 9, 2019

Conversation

ivanbasov
Copy link
Contributor

No description provided.

@ivanbasov ivanbasov requested review from tmat and jcouv June 26, 2019 01:17
@ivanbasov ivanbasov requested a review from a team as a code owner June 26, 2019 01:17
@ivanbasov ivanbasov added the Test Test failures in roslyn-CI label Jun 26, 2019
}

[Fact]
public void ChangeArrayOfNullableToArrayOfNonNullable()
Copy link
Member

@tmat tmat Jul 8, 2019

Choose a reason for hiding this comment

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

ChangeArrayOfNullableToArrayOfNonNullable [](start = 20, length = 41)

The foreach tests do not test anything interesting as they are written. We already tested changing the type of a local variable definition above.

What would be interesting to test in foreach loop is when the active statement is inside the loop (e.g. in {) and the type of arr is changed at that point (either explicitly or implicitly via non-nullable inference). We also don't need to test all combinations of nullable to non-nullable in that case. One is enough.

#Resolved

Copy link
Member

Choose a reason for hiding this comment

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

It would be better to add such foreach tests to the ForEach Statement section since foreach is the major feature EnC-wise and nullability is just one of the ways the type of the iterator may change.


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

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

🕐

edits.VerifySemanticDiagnostics(
// (9,25): error CS8652: The feature 'nullable reference types' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version.
// static void M(string? x)
Diagnostic(ErrorCode.ERR_FeatureInPreview, "?").WithArguments("nullable reference types").WithLocation(9, 25));
Copy link
Member

@tmat tmat Jul 8, 2019

Choose a reason for hiding this comment

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

What's the point of checking the error? You should set the test up so that the error is not reported. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, total mess. Sorry!


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

#endregion
#endregion

#region Nullables
Copy link
Member

@tmat tmat Jul 8, 2019

Choose a reason for hiding this comment

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

#region Nullables [](start = 7, length = 18)

These tests are not particularly useful here. What part of the EnC logic are they testing?

They would rather be useful in compilers where we test local slot and lambda mapping - see e.g. ComplexTypes test in LocalSlotMappingTests, MethodWithClosure1 in EditAndContinueClosureTests, EditAndContinueStateMachineTests for await/iterator testing etc. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. Will send a separate PR with compiles tests.


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

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

@ivanbasov ivanbasov merged commit 87b5fed into dotnet:master Jul 9, 2019
@ivanbasov ivanbasov deleted the nullableediting branch July 9, 2019 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Interactive-EnC New Language Feature - Nullable Reference Types Nullable Reference Types Test Test failures in roslyn-CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants