Skip to content

Conversation

@AtolagbeMuiz
Copy link
Contributor

This Pull Request fixes Assert.ContainsSingle should accept non-generic collections #6732

This Implementation involves an overload being added to the Assert.ContainsSingle which allows non-generic collections like ArrayList.

Current Behaviour

Assert.ContainsSingle throws compile time error because it only accepts generic collections.
Screenshot 2025-10-12 230729

Expected Behaviour

With this Implementation, Assert.ContainsSingle will allow non-generic collection as seen below;

image image

@AtolagbeMuiz
Copy link
Contributor Author

@Youssef1313 do we need to hndle AssertSingleInterpolatedStringHandler for non-generic collection as well

public static object ContainsSingle(IEnumerable collection, [InterpolatedStringHandlerArgument(nameof(collection))] ref AssertSingleInterpolatedStringHandler message, [CallerArgumentExpression(nameof(collection))] string collectionExpression = "") => message.ComputeAssertion(collectionExpression);

…rpolationStringHandler based off Assert.HasCount
@AtolagbeMuiz
Copy link
Contributor Author

removed the AssertSingleInterpolatedStringHandler for non-generic implemnation based because we didnt implement if for Assert.Count as well....

@Youssef1313

@Youssef1313 Youssef1313 self-assigned this Oct 17, 2025
@Youssef1313 Youssef1313 self-requested a review October 17, 2025 23:36
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

For the 2 new APIs, we could instead call the generic version by doing .Cast<object>(). There is a potential small performance hit but I think it's ok.

WDYT @Youssef1313

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

Reviving my earlier comment, I would personally use Cast<object> to avoid to have to maintain both implementations, the perf cost is IMO neglictible.

Comment on lines +362 to +372
public void ContainsSingle_InNonGenericCollection_NoMessage_WithEmptyCollection_ReturnsNoElement()
{
// Arrange
var collection = new ArrayList();

// Act
Action action = () => Assert.ContainsSingle(collection);

// Assert
action.Should().Throw<AssertFailedException>();
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove this test as it has no value compared to test below (besides test comment is wrong).

Comment on lines +377 to +387
public void ContainsSingle_InNonGenericCollection_AssertCustomMessage_WithEmptyCollection_ThrowsException()
{
// Arrange
var collection = new ArrayList();

// Act
Action action = () => Assert.ContainsSingle(collection);

// Assert
action.Should().Throw<AssertFailedException>().WithMessage("Assert.ContainsSingle failed. Expected collection to contain exactly one element but found 0 element(s). 'collection' expression: 'collection'.");
}
Copy link
Member

Choose a reason for hiding this comment

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

Please add another test case like:

 public void ContainsSingle_InNonGenericCollection_AssertCustomMessage_WithEmptyCollection_ThrowsException()
    {
        // Arrange
        var collection = new ArrayList();

        // Act
        Action action = () => Assert.ContainsSingle(collection, "my custom message");

        // Assert
        action.Should().Throw<AssertFailedException>().WithMessage("Assert.ContainsSingle failed. Expected collection to contain exactly one element but found 0 element(s). 'collection' expression: 'collection'.");
Comment view    }

@Youssef1313
Copy link
Member

@Evangelink I would personally avoid .Cast<object>. It might fail scenarios like:

unsafe
{
    var x = new int*[] { (int*)1 };
    IEnumerable y = x;
    Assert.ContainsSingle(y);
}

It might also be worth adding such test :)

@Youssef1313
Copy link
Member

Actually the scenario above might be invalid either way.

/// <returns>The item that matches the predicate.</returns>
public static object ContainsSingle(Func<object, bool> predicate, IEnumerable collection, string? message = "", [CallerArgumentExpression(nameof(predicate))] string predicateExpression = "", [CallerArgumentExpression(nameof(collection))] string collectionExpression = "")
{
var matchingElements = new List<object>();
Copy link
Member

Choose a reason for hiding this comment

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

Are we creating a list unnecessary? We only care about first match and acount.

/// Users shouldn't pass a value for this parameter.
/// </param>
/// <returns>The item.</returns>
public static object ContainsSingle(IEnumerable collection, string? message = "", [CallerArgumentExpression(nameof(collection))] string collectionExpression = "")
Copy link
Member

Choose a reason for hiding this comment

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

This method can be written in terms of the predicate method, where the predicate is just static _ => true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assert.ContainsSingle should accept non-generic collections

3 participants