Path segment bug (or test bug, choose your own adventure!)

[daveaglick]

Found a bug in either the Cake.Core.IO.Path segments calculation or the PathTests.TheSegmentsProperty.Should_Return_Segments_Of_Path() test.

The test is a theory, but the data isn't used in the test - instead it uses a hardcoded path:

[Theory]
[InlineData("Hello/World")]
[InlineData("/Hello/World")]
[InlineData("/Hello/World/")]
[InlineData("./Hello/World/")]
public void Should_Return_Segments_Of_Path(string pathName)
{
    // Given
    var path = new TestingPath("Hello/World");

    // When, Then
    Assert.Equal(2, path.Segments.Length);
    Assert.Equal("Hello", path.Segments[0]);
    Assert.Equal("World", path.Segments[1]);
}

So that's obviously wrong. When you change the test to use pathName, "/Hello/World" and "/Hello/World/" fail because the segment logic looks like this:

_segments = _path.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries);
if (_path.StartsWith("/") && _segments.Length > 0)
{
    _segments[0] = "/" + _segments[0];
}

I.e., if the path starts with "/", then "/" is prepended to the first segment.

So either the test is wrong or the logic is wrong and it just wasn't showing up because the test was wrong. I can take care of it - just need guidance on what the intent is here. Which is correct, the test or the logic?

[patriksvensson]

@daveaglick Oh, interesting. I have to dig into the source code to answer this properly.

[patriksvensson]

@daveaglick The test is theoretically correct. The reason for prepending the slash is to be able to differentiate between an absolute path from a relative one when working only with the segments.

There are no side effects from having the slash there, but it's not pretty and theoretically incorrect. I removed the part that appends the slash to see what was affected and 1500 tests failed 😄 Most of the tests fail because the FakeFileSystem relies on the segments to be prefixed with the slash so it's fixable, but I think it's better to leave it as it is at the moment and fix the tests to make it clearer in the future that this is "by design".

Did you encounter a bug in Wyam because of this?

[Theory]
[InlineData("Hello/World")]
[InlineData("./Hello/World/")]
public void Should_Return_Segments_Of_Path(string pathName)
{
    // Given
    var path = new TestingPath(pathName);

    // When, Then
    Assert.Equal(2, path.Segments.Length);
    Assert.Equal("Hello", path.Segments[0]);
    Assert.Equal("World", path.Segments[1]);
}

[Theory]
[InlineData("/Hello/World")]
[InlineData("/Hello/World/")]
public void Should_Return_Segments_Of_Path_And_Leave_Absolute_Directory_Separator_Intact(string pathName)
{
    // Given
    var path = new TestingPath(pathName);

    // When, Then
    Assert.Equal(2, path.Segments.Length);
    Assert.Equal("/Hello", path.Segments[0]);
    Assert.Equal("World", path.Segments[1]);
}

[daveaglick]

1500 tests failed

Doh! It's never easy, is it.

Did you encounter a bug in Wyam because of this

Not at all - I was just copying over some tests and converting to NUnit when I noticed this one wasn't using the theory data. Figured it was an easy fix, but then the two tests failed and down the rabbit hole.

PR incoming...

[daveaglick]

BTW, for what it's worth I agree with the leading slash in the segments being theoretically incorrect. I had actually already removed it in my port. Given there's already a IsRelative property it felt redundant.

[patriksvensson]

@daveaglick Yes, I agree. The IsRelative part should be enough and we will remove it as well eventually. Just want to be really sure we don't break anything first 😄