Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Add TryAdd and Clear regression tests #32407

Merged
merged 7 commits into from
Sep 25, 2018
Merged

Add TryAdd and Clear regression tests #32407

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
80ddf80
Add TryAdd and Clear regression tests
A-And Sep 22, 2018
6b4248d
Add Run Condition on Clear()
A-And Sep 22, 2018
bcf1258
Address PR Feedback
A-And Sep 24, 2018
1c20776
Address PR Feedback #2
A-And Sep 24, 2018
7730b1d
Address PR Feedback #3
A-And Sep 24, 2018
05d30db
Remove Extra Line
A-And Sep 24, 2018
7075156
Add MoveNext Result Asserts
A-And Sep 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions src/System.Collections/tests/Generic/Dictionary/Dictionary.Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,21 @@ public void IDictionary_NonGeneric_Contains_KeyOfWrongType(int count)
}
}


[Fact]
public void Clear_OnEmptyCollection_DoesNotInvalidateEnumerator()
{
if (ModifyEnumeratorAllowed.HasFlag(ModifyOperation.Clear))
{
IDictionary dictionary = new Dictionary<string, string>();
IEnumerator valuesEnum = dictionary.GetEnumerator();

dictionary.Clear();
Assert.Empty(dictionary);
valuesEnum.MoveNext();
Copy link
Member

Choose a reason for hiding this comment

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

Assert the expected result of MoveNext?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@stephentoub stephentoub Sep 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Author

@A-And A-And Sep 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Either way, I'm deferring to you - added.

}
}

#endregion

#region ICollection tests
Expand Down Expand Up @@ -250,6 +265,18 @@ public void Remove_NonExistentEntries_DoesNotPreventEnumeration()
}
}

[Fact]
public void Unsuccessful_TryAdd_DoesNotInvalidateEnumerator()
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd suggest a name more like "TryAdd_ItemAlreadyExists_DoesNotInvalidEnumerator".

{
var dictionary = new Dictionary<string, string>();
dictionary.Add("a", "b");

var valuesEnum = dictionary.GetEnumerator();
Copy link

Choose a reason for hiding this comment

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

nit : change var here to IEnumerator

Assert.False(dictionary.TryAdd("a", "c"));

valuesEnum.MoveNext();
Copy link
Member

Choose a reason for hiding this comment

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

Assert the expected result of MoveNext?

}

[Theory]
[MemberData(nameof(CopyConstructorInt32Data))]
public void CopyConstructorInt32(int size, Func<int, int> keyValueSelector, Func<IDictionary<int, int>, IDictionary<int, int>> dictionarySelector)
Expand Down