ConcurrentDictionary Test Coverage | TestGetOrAddOrUpdate

[Notheisz57]

Code for context:

runtime/src/libraries/System.Collections.Concurrent/tests/ConcurrentDictionary/ConcurrentDictionaryTests.cs

Lines 485 to 591 in 86550d7

	[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
	public static void TestGetOrAdd()
	{
	TestGetOrAddOrUpdate(1, 1, 1, 10000, true);
	TestGetOrAddOrUpdate(5, 1, 1, 10000, true);
	TestGetOrAddOrUpdate(1, 1, 2, 5000, true);
	TestGetOrAddOrUpdate(1, 1, 5, 2000, true);
	TestGetOrAddOrUpdate(4, 0, 4, 2000, true);
	TestGetOrAddOrUpdate(16, 31, 4, 2000, true);
	TestGetOrAddOrUpdate(64, 5, 5, 5000, true);
	TestGetOrAddOrUpdate(5, 5, 5, 25000, true);
	}
	[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
	public static void TestAddOrUpdate()
	{
	TestGetOrAddOrUpdate(1, 1, 1, 10000, false);
	TestGetOrAddOrUpdate(5, 1, 1, 10000, false);
	TestGetOrAddOrUpdate(1, 1, 2, 5000, false);
	TestGetOrAddOrUpdate(1, 1, 5, 2000, false);
	TestGetOrAddOrUpdate(4, 0, 4, 2000, false);
	TestGetOrAddOrUpdate(16, 31, 4, 2000, false);
	TestGetOrAddOrUpdate(64, 5, 5, 5000, false);
	TestGetOrAddOrUpdate(5, 5, 5, 25000, false);
	}
	private static void TestGetOrAddOrUpdate(int cLevel, int initSize, int threads, int addsPerThread, bool isAdd)
	{
	ConcurrentDictionary<int, int> dict = new ConcurrentDictionary<int, int>(cLevel, 1);
	int count = threads;
	using (ManualResetEvent mre = new ManualResetEvent(false))
	{
	for (int i = 0; i < threads; i++)
	{
	int ii = i;
	Task.Run(
	() =>
	{
	for (int j = 0; j < addsPerThread; j++)
	{
	if (isAdd)
	{
	//call one of the overloads of GetOrAdd
	switch (j % 3)
	{
	case 0:
	dict.GetOrAdd(j, -j);
	break;
	case 1:
	dict.GetOrAdd(j, x => -x);
	break;
	case 2:
	dict.GetOrAdd(j, (x,m) => x * m, -1);
	break;
	}
	}
	else
	{
	switch (j % 3)
	{
	case 0:
	dict.AddOrUpdate(j, -j, (k, v) => -j);
	break;
	case 1:
	dict.AddOrUpdate(j, (k) => -k, (k, v) => -k);
	break;
	case 2:
	dict.AddOrUpdate(j, (k,m) => k*m, (k, v, m) => k * m, -1);
	break;
	}
	}
	}
	if (Interlocked.Decrement(ref count) == 0) mre.Set();
	});
	}
	mre.WaitOne();
	}
	foreach (var pair in dict)
	{
	Assert.Equal(pair.Key, -pair.Value);
	}
	List<int> gotKeys = new List<int>();
	foreach (var pair in dict)
	gotKeys.Add(pair.Key);
	gotKeys.Sort();
	List<int> expectKeys = new List<int>();
	for (int i = 0; i < addsPerThread; i++)
	expectKeys.Add(i);
	Assert.Equal(expectKeys.Count, gotKeys.Count);
	for (int i = 0; i < expectKeys.Count; i++)
	{
	Assert.True(expectKeys[i].Equals(gotKeys[i]),
	string.Format("* Test '{4}': Level={0}, initSize={1}, threads={2}, addsPerThread={3})" + Environment.NewLine +
	"> FAILED. The set of keys in the dictionary is are not the same as the expected.",
	cLevel, initSize, threads, addsPerThread, isAdd ? "GetOrAdd" : "GetOrUpdate"));
	}
	// Finally, let's verify that the count is reported correctly.
	Assert.Equal(addsPerThread, dict.Count);
	Assert.Equal(addsPerThread, dict.ToArray().Length);
	}

While the other switch-case statements make use of the lambda arguments, the case statement on line 547 does not:

runtime/src/libraries/System.Collections.Concurrent/tests/ConcurrentDictionary/ConcurrentDictionaryTests.cs

Line 547 in 86550d7

dict.AddOrUpdate(j, -j, (k, v) => -j);

Should this test case be more thorough/complete in testing these overloads and their lambda arguments, or is this test case good enough as it is?

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Code for context:

runtime/src/libraries/System.Collections.Concurrent/tests/ConcurrentDictionary/ConcurrentDictionaryTests.cs

Lines 485 to 591 in 86550d7

	[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
	public static void TestGetOrAdd()
	{
	TestGetOrAddOrUpdate(1, 1, 1, 10000, true);
	TestGetOrAddOrUpdate(5, 1, 1, 10000, true);
	TestGetOrAddOrUpdate(1, 1, 2, 5000, true);
	TestGetOrAddOrUpdate(1, 1, 5, 2000, true);
	TestGetOrAddOrUpdate(4, 0, 4, 2000, true);
	TestGetOrAddOrUpdate(16, 31, 4, 2000, true);
	TestGetOrAddOrUpdate(64, 5, 5, 5000, true);
	TestGetOrAddOrUpdate(5, 5, 5, 25000, true);
	}
	[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
	public static void TestAddOrUpdate()
	{
	TestGetOrAddOrUpdate(1, 1, 1, 10000, false);
	TestGetOrAddOrUpdate(5, 1, 1, 10000, false);
	TestGetOrAddOrUpdate(1, 1, 2, 5000, false);
	TestGetOrAddOrUpdate(1, 1, 5, 2000, false);
	TestGetOrAddOrUpdate(4, 0, 4, 2000, false);
	TestGetOrAddOrUpdate(16, 31, 4, 2000, false);
	TestGetOrAddOrUpdate(64, 5, 5, 5000, false);
	TestGetOrAddOrUpdate(5, 5, 5, 25000, false);
	}
	private static void TestGetOrAddOrUpdate(int cLevel, int initSize, int threads, int addsPerThread, bool isAdd)
	{
	ConcurrentDictionary<int, int> dict = new ConcurrentDictionary<int, int>(cLevel, 1);
	int count = threads;
	using (ManualResetEvent mre = new ManualResetEvent(false))
	{
	for (int i = 0; i < threads; i++)
	{
	int ii = i;
	Task.Run(
	() =>
	{
	for (int j = 0; j < addsPerThread; j++)
	{
	if (isAdd)
	{
	//call one of the overloads of GetOrAdd
	switch (j % 3)
	{
	case 0:
	dict.GetOrAdd(j, -j);
	break;
	case 1:
	dict.GetOrAdd(j, x => -x);
	break;
	case 2:
	dict.GetOrAdd(j, (x,m) => x * m, -1);
	break;
	}
	}
	else
	{
	switch (j % 3)
	{
	case 0:
	dict.AddOrUpdate(j, -j, (k, v) => -j);
	break;
	case 1:
	dict.AddOrUpdate(j, (k) => -k, (k, v) => -k);
	break;
	case 2:
	dict.AddOrUpdate(j, (k,m) => k*m, (k, v, m) => k * m, -1);
	break;
	}
	}
	}
	if (Interlocked.Decrement(ref count) == 0) mre.Set();
	});
	}
	mre.WaitOne();
	}
	foreach (var pair in dict)
	{
	Assert.Equal(pair.Key, -pair.Value);
	}
	List<int> gotKeys = new List<int>();
	foreach (var pair in dict)
	gotKeys.Add(pair.Key);
	gotKeys.Sort();
	List<int> expectKeys = new List<int>();
	for (int i = 0; i < addsPerThread; i++)
	expectKeys.Add(i);
	Assert.Equal(expectKeys.Count, gotKeys.Count);
	for (int i = 0; i < expectKeys.Count; i++)
	{
	Assert.True(expectKeys[i].Equals(gotKeys[i]),
	string.Format("* Test '{4}': Level={0}, initSize={1}, threads={2}, addsPerThread={3})" + Environment.NewLine +
	"> FAILED. The set of keys in the dictionary is are not the same as the expected.",
	cLevel, initSize, threads, addsPerThread, isAdd ? "GetOrAdd" : "GetOrUpdate"));
	}
	// Finally, let's verify that the count is reported correctly.
	Assert.Equal(addsPerThread, dict.Count);
	Assert.Equal(addsPerThread, dict.ToArray().Length);
	}

While the other switch-case statements make use of the lambda arguments, the case statement on line 547 does not:

runtime/src/libraries/System.Collections.Concurrent/tests/ConcurrentDictionary/ConcurrentDictionaryTests.cs

Line 547 in 86550d7

dict.AddOrUpdate(j, -j, (k, v) => -j);

Should this test case be more thorough/complete in testing these overloads and their lambda arguments, or is this test case good enough as it is?

Author:	Notheisz57
Assignees:	-
Labels:	area-System.Collections
Milestone:	-

[stephentoub]

Should this test case be more thorough/complete in testing these overloads and their lambda arguments, or is this test case good enough as it is?

Can you come up with a change to the source code for ConcurrentDictionary that would break its functionality but it would still pass all of these tests?

This issue has been marked needs-author-action and may be missing some important information.

[Notheisz57]

This seems to pass the existing tests in System.Collections.Concurrent.Tests without being too contrived:

Code for context:

runtime/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs

Lines 1425 to 1473 in 86550d7

	public TValue AddOrUpdate(TKey key, TValue addValue, Func<TKey, TValue, TValue> updateValueFactory)
	{
	if (key is null)
	{
	ThrowHelper.ThrowKeyNullException();
	}
	if (updateValueFactory is null)
	{
	ThrowHelper.ThrowArgumentNullException(nameof(updateValueFactory));
	}
	Tables tables = _tables;
	IEqualityComparer<TKey>? comparer = tables._comparer;
	int hashcode = GetHashCode(comparer, key);
	while (true)
	{
	if (TryGetValueInternal(tables, key, hashcode, out TValue? oldValue))
	{
	// key exists, try to update
	TValue newValue = updateValueFactory(key, oldValue);
	if (TryUpdateInternal(tables, key, hashcode, newValue, oldValue))
	{
	return newValue;
	}
	}
	else
	{
	// key doesn't exist, try to add
	if (TryAddInternal(tables, key, hashcode, addValue, updateIfExists: false, acquireLock: true, out TValue resultingValue))
	{
	return resultingValue;
	}
	}
	if (tables != _tables)
	{
	tables = _tables;
	if (!ReferenceEquals(comparer, tables._comparer))
	{
	comparer = tables._comparer;
	hashcode = GetHashCode(comparer, key);
	}
	}
	}
	}

This line:

runtime/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs

Line 1447 in 86550d7

TValue newValue = updateValueFactory(key, oldValue);

Changed to something like this:

TValue newValue = updateValueFactory(key, addValue);

When key is already present, updateValueFactory would receive addValue instead of the existing oldValue.

[stephentoub]

If that's the case then please feel free to submit a PR that augments the tests in a way where they'd fail if that broke.