Incorrect solutions passing tests (in C# track)

Hi,

today in my mentoring session I discovered that one of the exercises in c# track pass all the tests but produce code that couldn’t be used in production. The problem is in the tests that pass through wrong solutions. I have solved the problem myself and somehow I ended up in similarly wrong solution! So I started going through community solution to discover that many of them have the same problem.

@ErikSchierboom , may I add tests to strengthen the test suite? What is the current thinking about updates to the track? Are PRs for C# track suspended or expected right now?

1 Like

Sounds like a good PR. Maybe tracks for languages with appropriate tools could be subjected to some Mutation Testing. :-)

Would you mind sharing what incorrect solution passes the tests? Adding additional tests should be accompanied with an explanation of what specific problem the tests serve to prevent, and I’m not seeing any specifics here.

Note, this isn’t necessarily an issue. For example, the list operations exercise asks you to reimplement lists. In most languages, lists is a pretty basic language feature and reimplementing basic features is terrible for production code.

1 Like

Thanks @IsaacG,

And this stage my specific question is whether PRs are at the moment accepted or not. (see the blog post on the subject).

Perparing detailed explanation of the problem and potential solution takes effort which I’m happy to put in, but only if there is a chance that it will be of any value.

Depending on the issue, this may be specific to C# or it might be a test stemming from the problem specs. The problem specs repo is open to changes if there is a specific and clear reason for a change. I’m not a maintainer of the C# track but I suspect that the track maintainers may be open to fixes when there’s a clear and demonstrable issue. Some details are helpful to decide if this specific issue warrants a change.

@michalporeba Suggestions of the following form are almost always welcome, and need not be a lot of work to make:

Working on solving Exercise on the Language track I accidentally produced a false possitive:

optionally include faulty but accepted (partial) solution

The problem with this solution is that it does Wrong Thing Described In One Sentence. Looking through the community solutions I found that my mistake is common: example link, example link, example link. I think we might be able to catch all of these by expanding the test suite to include a test along the lines of

loose draft of a test, not necessarily entirely correct yet

Would there be interest in adding such a test?

If it turns out there is interest, then the real work can begin.

The exercise is Simple Linked List. The core of the issue is specific to C# track, that’s why I put it in Programming > C# part of the forum. But now that I looked more into the issue there are more interesting quirks to it, some of them might be of interest to people maintaining problem specifications / stories.

Problem 1 - C# and IEnumerable<T> implementation

The task requires implementation of IEnumerable<T> interface - a very common thing C#. But the test for it is not adequate.

[Fact]
public void Implements_enumerable() {
     var values = new SimpleLinkedList<int>(2).Add(1);
     Assert.Equal(new[] { 2, 1 }, values);
}

A common implementation is something like this:

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;
  }
}

And the above test will pass, however if we extend it by just adding one extra number it will fail, as the GetEnumerator() will return the first and the last element from the list.

Pull request with a proposed fix (2098).

Problem 2 - dissimilarity between instructions and tests

The instructions contain this fragment:

This variant of linked lists is often used to represent sequences or push-down stacks (also called a LIFO stack; Last In, First Out).

However, the tests require First in, First Out structure. In many cases, including the snipped shown above the tests expect FIFO behaviour which may be confusing because of the dissimilarities between instructions and expectations.

I don’t see an obvious, low impact solution.

  1. Change description - lower impact on existing solutions but will go against current efforts to standardise the problem statements
  2. Change the tests to expect LIFO behaviour will invalidate all submitted solutions

To make it more obvious Remove method could be added so in tests it is obvious where after adding 1, 2, 3 in that order whether 1 or 3 should be returned first.

Problem 3 - hints with malformed link.

The instructions.append.md has a malformed link.

Pull request to fix the issue (2099).

Something else to consider

Some of the other language tracks I have looked at implement a different type of simple linked list, one that consists of the wrapping type and a node type. This allows for more robust solutions. Adding and LIFO behaviour without problems with hanging references because the external reference is always to a wrapper not to a specific element that might be removed.

@MatthijsBlom in principle it is easy, however in forum we lack context as the code is somewhere else, and so to prepare the examples I might as well write the code. The real work in those cases is to commit and create a PR, which would have very similar text to the forum post.

Good catch @michalporeba! We should indeed be verifying this in a different way. As you’ve already submitted a PR, I’ll add my suggestion there. It would have been fine to discuss a possible solution here on the forum, as you can expect us (the maintainers) to know the code base well enough to be able to discuss it here.

@michalporeba I’ve reviewed the PR!

Hi @ErikSchierboom

Thanks, I’ll comment more on the specific in the PR.

That probably would be a single sided discussion because the rest of us (non-maintainers) don’t have the same level of code base knowledge. To be able to express ideas about the code I don’t know well I really have to read and consider the code first, which I cannot do here. (not complaining, just offering a different perspective to consider)

I think this is the sanest option. Yes, it will invalidate submitted solutions, but it’ll be much clearer for future students.

What would this look like?

I’ll prepare an example in the evening.

1 Like

I was just talking with Bethany about how it would be nice to add test data to the problem spec for this exercise.

A rough draft is available on github in a pull request. It is just a draft and will require more work, but the intention is to further the discussion for now rather than present code for review.