Improve ConcurrentDictionary test coverage by asserting callback arguments

[Copilot]

Description

The TestGetOrAddOrUpdate test wasn't fully exercising the callback delegates in GetOrAdd and AddOrUpdate overloads. The lambdas either ignored their arguments or used them without verifying correctness, meaning a bug in how arguments are passed to the delegates would go undetected.

Added explicit assertions inside all callbacks to verify that the correct values are passed:

GetOrAdd callbacks:

// Before: no verification of argument values
dict.GetOrAdd(j, x => -x);
dict.GetOrAdd(j, (x, m) => x * m, -1);

// After: assertions verify expected values
dict.GetOrAdd(j, x =>
{
    Assert.Equal(j, x);
    return -x;
});
dict.GetOrAdd(j, (x, m) =>
{
    Assert.Equal(j, x);
    Assert.Equal(-1, m);
    return x * m;
}, -1);

AddOrUpdate callbacks:

// Before: lambda ignores arguments
dict.AddOrUpdate(j, -j, (k, v) => -j);

// After: assertions verify key, old value, and factory argument
dict.AddOrUpdate(j, -j, (k, v) =>
{
    Assert.Equal(j, k);
    Assert.Equal(-j, v);
    return v;
});
dict.AddOrUpdate(j, k =>
{
    Assert.Equal(j, k);
    return -k;
}, (k, v) =>
{
    Assert.Equal(j, k);
    Assert.Equal(-j, v);
    return -k;
});
dict.AddOrUpdate(j, (k, m) =>
{
    Assert.Equal(j, k);
    Assert.Equal(-1, m);
    return k * m;
}, (k, v, m) =>
{
    Assert.Equal(j, k);
    Assert.Equal(-j, v);
    Assert.Equal(-1, m);
    return k * m;
}, -1);

This ensures that if there was a bug where any callback argument wasn't properly passed, the test would catch it with an explicit assertion failure.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>ConcurrentDictionary Test Coverage | TestGetOrAddOrUpdate</issue_title>
<issue_description>> 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?</issue_description>

Comments on the Issue (you are @copilot in this section)

@ Tagging subscribers to this area: @dotnet/area-system-collections See info in [area-owners.md](https://github.com/dotnet/runtime/blob/main/docs/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?</comment_new>
<comment_new>@
This issue has been marked needs-author-action and may be missing some important information.</comment_new>
<comment_new>@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.</comment_new>

• Fixes ConcurrentDictionary Test Coverage | TestGetOrAddOrUpdate #83429

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[Copilot]

Pull request overview

This PR improves test coverage for ConcurrentDictionary.AddOrUpdate by ensuring that the updateValueFactory delegate's parameters are properly tested. The change modifies a lambda expression in the TestGetOrAddOrUpdate test method to use its old value argument instead of ignoring it.

Changes:

• Modified the lambda in case 0 of the AddOrUpdate test to use the old value parameter v instead of ignoring it and using the captured variable -j

[stephentoub]

LGTM if CI passes

[stephentoub]

/ba-g timeout