Add TryAdd and Clear regression tests #32407
Conversation
A-And
force-pushed
the
DictRegressionTests
branch
from
e373d99 to
f651867
Compare
A-And
force-pushed
the
DictRegressionTests
branch
from
f651867 to
9406de2
Compare
A-And
force-pushed
the
DictRegressionTests
branch
from
9406de2 to
6b4248d
Compare
| [Fact] | ||
| public void Clear_OnEmptyCollection_DoesNotInvalidateEnumerator() | ||
| { | ||
| if (PlatformDetection.IsFullFramework) |
There was a problem hiding this comment.
use [SkipOnTargetFramework(~TargetFrameworkMonikers.NetFramework)]
Can use please also add the reason why we are skipping this test for .net core
There was a problem hiding this comment.
The implementation of Clear was changed recently to bring it in line with the behavior of Remove and not invalidate the enumerator. The if() clause here is wrong.
PlatformDetection is definitely unnecessary here. Instead the test should probably rely on expected behavior. Moved test to Dictionary_IDictionary_NonGeneric_Tests and instead inspect ModifyEnumeratorSucceeds for expected behavior.
| Assert.Empty(dictionary); | ||
| valuesEnum.MoveNext(); | ||
| } | ||
|
|
| { | ||
| if (PlatformDetection.IsFullFramework) | ||
| { | ||
| Dictionary<string, string> dictionary = new Dictionary<string, string>(); |
There was a problem hiding this comment.
nit: left hand side can be just var (coding guidelines)
| if (PlatformDetection.IsFullFramework) | ||
| { | ||
| Dictionary<string, string> dictionary = new Dictionary<string, string>(); | ||
| var valuesEnum = dictionary.GetEnumerator(); |
There was a problem hiding this comment.
nit: concrete type on the left hand side instead of var (coding guidelines)
| [Fact] | ||
| public void Unsuccessful_TryAdd_DoesNotInvalidateEnumerator() | ||
| { | ||
| Dictionary<string, string> dictionary = new Dictionary<string, string>(); |
There was a problem hiding this comment.
nit: again var on the left hand side.
A-And
force-pushed
the
DictRegressionTests
branch
from
5d192ab to
bcf1258
Compare
| [Fact] | ||
| public void Clear_OnEmptyCollection_DoesNotInvalidateEnumerator() | ||
| { | ||
| if(ModifyEnumeratorAllowed.HasFlag(ModifyOperation.Clear)) |
| var dictionary = new Dictionary<string, string>(); | ||
| dictionary.Add("a", "b"); | ||
|
|
||
| var valuesEnum = dictionary.GetEnumerator(); |
|
@Anipik - thanks for the patience. I need to get my linter in order. |
|
Seeing some Microsoft.VisualBasic.Tests failures. @Anipik - is this expected? |
|
yes the failures were expected earlier in the morning today but they were reverted back by this #32439 |
|
@dotnet-bot test this please |
| } | ||
|
|
||
| [Fact] | ||
| public void Unsuccessful_TryAdd_DoesNotInvalidateEnumerator() |
There was a problem hiding this comment.
Nit: I'd suggest a name more like "TryAdd_ItemAlreadyExists_DoesNotInvalidEnumerator".
| IEnumerator valuesEnum = dictionary.GetEnumerator(); | ||
| Assert.False(dictionary.TryAdd("a", "c")); | ||
|
|
||
| valuesEnum.MoveNext(); |
There was a problem hiding this comment.
Assert the expected result of MoveNext?
|
|
||
| dictionary.Clear(); | ||
| Assert.Empty(dictionary); | ||
| valuesEnum.MoveNext(); |
There was a problem hiding this comment.
Assert the expected result of MoveNext?
There was a problem hiding this comment.
In this case MoveNext will always be false or thrown an InvalidOperationException.
It's probably better to be explicit, but wouldn't an Assert.False() here be a bit confusing? I.e. whether or not the enumerator advances or not is an implementation detail of IEnumerator we don't necessarily want to test with this.
There was a problem hiding this comment.
whether or not the enumerator advances or not is an implementation detail of IEnumerator we don't necessarily want to test with this.
How is it an implementation detail? Regardless of implementation, it should always return false, no?
There was a problem hiding this comment.
Absolutely true.
I suppose my confusion here stems from the fact that "invalidate the enumerator" here means not changing the state of the collection (and not throwing an InvalidOperationException). The functionality of Clear is already covered by other tests. Whether or not MoveNext is false or not doesn't necessarily matter for the purpose of this test.
There was a problem hiding this comment.
Either way, I'm deferring to you - added.
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
A few nits, otherwise LGTM.
* Add TryAdd and Clear regression tests * Add Run Condition on Clear() * Address PR Feedback * Address PR Feedback dotnet/corefx#2 * Address PR Feedback dotnet/corefx#3 * Remove Extra Line * Add MoveNext Result Asserts Commit migrated from dotnet/corefx@d8a0778
6 participants
Added regression tests for changes which were rolled back in dotnet/coreclr#17096 .
@jkotas @sergiy-k