improving test for GetEnumerable() in Simple Linked List

[michalporeba]

Exercise: Simple Linked List

Problem: A number of community solutions pass the existing tests, but fail to implement GetEnumerator() correctly. This is because the test case is too simple to validate the implementation.

This is an example of implementation that will pass the current test which tests only two items in the list, but will fail for any list longer than two elements.

public SimpleLinkedList<T> Add(T value)
{
  /* this is incorrect but enough for the tests to pass */
  this.next = new SimpleLinkedList<T>(value);
  return this;
}

public IEnumerator<T> GetEnumerator() {
  var current = this;
  while(current != null) { 
    yield return (T)current.Value;
    current = current.Next;
  }
}

Solution: The proposed change improves robustness of the test suit to ensure invalid solutions are not marked as passed.

See: Forum discussion.

[github-actions]

Hello. Thanks for opening a PR on Exercism. We are currently in a phase of our journey where we have paused community contributions to allow us to take a breather and redesign our community model. You can learn more in this blog post. As such, all issues and PRs in this repository are being automatically closed.

That doesn't mean we're not interested in your ideas, or that if you're stuck on something we don't want to help. The best place to discuss things is with our community on the Exercism Community Forum. You can use this link to copy this into a new topic there.

---

Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

[ErikSchierboom]

Thanks so much for this! I've left a comment with some suggestions.

[ErikSchierboom]

Having looked into this a bit, I think we should actually change things up a bit.
Let's actually verify that the student is implementing IEnumerable<int>:

Suggested change

	var values = new SimpleLinkedList<int>(2).Add(1).Add(3);
	Assert.Equal(new[] { 2, 1, 3 }, values);
	var list = new SimpleLinkedList<int>(2).Add(1);
	var enumerable = Assert.IsAssignableFrom<IEnumerable<int>>(list);
	var enumerator = enumerable.GetEnumerator();
	Assert.True(enumerator.MoveNext());
	Assert.Equal(2, enumerator.Current);
	Assert.True(enumerator.MoveNext());
	Assert.Equal(1, enumerator.Current);
	Assert.False(enumerator.MoveNext());

Doing it this way will verify the implementation whilst still allowing the tests to run if the student hasn't yet implemented IEnumerable<int>.

We should change some other tests too which are relying on IEnumerable<T> to be implemented.
First up, the `Reverse`` test:

[Fact]
public void Reverse()
{
    var list = new SimpleLinkedList<int>(new[] { 11, 7, 5, 3, 2 });
    var reversed = list.Reverse();
    Assert.Equal(2, reversed.Value);
    Assert.Equal(3, reversed.Next.Value);
    Assert.Equal(5, reversed.Next.Next.Value);
    Assert.Equal(7, reversed.Next.Next.Next.Value);
    Assert.Equal(11, reversed.Next.Next.Next.Next.Value);
}

I also think this test should be moved above the Implements_enumerable test now that it no longer depends on that.

The second test that needs to be changed is Roundtrip. I also think we should move away from a Theory to a Fact, which is better supported in the test runner.

[Fact]
public void Roundtrip()
{
    var input = new[] {11, 7, 5, 3, 2};
    var list = new SimpleLinkedList<int>(input);
    var enumerable = Assert.IsAssignableFrom<IEnumerable>(list);
    Assert.Equal(input, enumerable);
}

[michalporeba]

I like the idea of using Assert.IsAssignableFrom. A number of tests could be improve like that.
I still think that having three values in the list allows for a more robust test.

The testing of reverse connects to the bigger discussion about whether this list should be LIFO or FIFO. Reverse doesn't have to be directly implemented as it is provided by Enumerable. Testing it can help catch cases where somebody implements IEnumerable without using the base class. In other languages reversing a LIFO is an interesting problem and a classic algorithm to learn, but we cannot do that here as we don't ask for the Pop or Remove operation to be implemented. This links to Problem 2 in the forum post and also the something else to consider section.

[ErikSchierboom]

I still think that having three values in the list allows for a more robust test.

Yeah sure! I hadn't even noticed that.

[ErikSchierboom]

I've left some comments on the forum post

[michalporeba]

@ErikSchierboom now that the other PR is merged in this one doesn't make any sense. I'll close it.