DeploymentItem is not os-independant in combination with folders

[smorokin]

Describe the bug

Hi,

I hope I am at the right repository for this. If not, please point me to the right one.
DeploymentItem is not os-independant when using it with folders. The folder name must end with /on linux and \on windows.

Thank you.

Steps To Reproduce

Example files:

TestFiles1/some_file1
TestFiles2/some_file2
MyTests.cs
MyTests.csproj

// MyTests.cs
[TestClass]
public class MyTests
{
    [TestMethod]
    [DeploymentItem(@"TestFiles1/")]
    public void TestFailsOnWindows()
    {
        var fs = File.Open(@"some_file1", FileMode.Open);
    }
    [TestMethod]
    [DeploymentItem(@"TestFiles2\")]
    public void TestFailsOnLinuxMac()
    {
        var fs = File.Open(@"some_file2", FileMode.Open);
    }
}

<!--MyTests.csproj-->
<!--...-->
  <ItemGroup>
    <Content Include="TestFiles1\**\*">
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </Content>
    <Content Include="TestFiles2\**\*">
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </Content>
  </ItemGroup>

Expected behavior

Using either \ or / on windows, linux or mac should always work.

Actual behavior

Files are not copied to the test directory and the test fails if the wrong format is used.

Additional context

None.

[Evangelink]

Hi @smorokin, I confirm that I can reproduce the issue even with version 3.0.2.

[smorokin]

I can also reproduce it with version 3.0.3.

[Evangelink]

You are right, this wasn't yet addressed. I don't have any ETA at the moment as we are dealing with many changes of priorities but will get back to you when possible.

[smorokin]

@Evangelink : Sorry, I did not want to bother you. Just tried the new version today and thought I document it here.

[Evangelink]

No worries @smorokin and sorry if I gave the impression to be annoyed by your comment.

[smorokin]

My impression was more "stressed" :-). Thank you for your work!

[smorokin]

Thank you @Evangelink !

[Evangelink]

We don't yet have any ETA for 3.1 but I hope to have it released during the summer (July or August).

[smorokin]

Don't worry. I have a working workaround (that throws warnings, but we ignore them). Just annotate twice with '/' and '':

[TestMethod]
[DeploymentItem(@"TestFiles1\")]
[DeploymentItem(@"TestFiles1/")]
public void TestFailsOnWindows()
{
    var fs = File.Open(@"some_file1", FileMode.Open);
}

[Evangelink]

From my tests it seems that you could simply get rid of the trailing separator. MSTest always tries to first find folder matching the given name and then files.

[smorokin]

For folders this should work, yes. We have also the case of

[DeploymentItem(@"TestFiles1\test.txt")]
[DeploymentItem(@"TestFiles1/test.txt")]

where both are neccessary.

[Evangelink]

I am reopening the issue to confirm this use case is also working (the deployment item part is not well tested).

[smorokin]

Hi @evangelik, sorry to bother you again, but there is still a problem with 3.1.1, but I am not sure if it is possible to improve the solution
As far as I understand there is a general problem with mixing linux and windows paths. Just replacing /
with \ and vice versa will not work in the general case (one example would be /tmp/some\ stuff which is a common valid path under linux, but some\stuff is also a single folder).
The current version does seem to work correctly if using only / on windows and linux, so that is what we will be using to avoid the test deploy warnings
PS:
I've seen you used TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) to remove a trailing slash, but both Path.DirectorySeparatorChar and Path.AltDirectorySeparatorChar resolve to / on linux (see doc). Hardcoding seems to be the better solution.

Also it seems that your CI pipeline does never run tests under linux, otherwise this test you added should have failed. I suspect that there might be more bugs hidden by this.

[Evangelink]

Also it seems that your CI pipeline does never run tests under linux, otherwise this test you added should have failed. I suspect that there might be more bugs hidden by this.

You are right! This is a chore task I want to work on for some time now but it's sadly always getting unprioritized. It's not super complex but requires a bit of work as the project itself doesn't build on Linux so we would need either some partial build or to retrieve the binaries built on windows to run the (non-Windows specifics) tests on Linux!

I've seen you used TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) to remove a trailing slash, but both Path.DirectorySeparatorChar and Path.AltDirectorySeparatorChar resolve to / on linux (see doc). Hardcoding seems to be the better solution.

Oh right, I definitely didn't consider all cases there... I am opening a ticket for that part (see #1730).

As far as I understand there is a general problem with mixing linux and windows paths. Just replacing /
with \ and vice versa will not work in the general case (one example would be /tmp/some\ stuff which is a common valid path under linux, but some\stuff is also a single folder).

Indeed that seems like a complex scenario... At the same time if "some\ stuff" is supposed to be the folder to copy that means this is something that will work only on Linux so there is no big issue. Maybe we could introduce some escaping, if you use the escaping then we don't normalize the path.

[smorokin]

Thank you for addressing this.

You are right! This is a chore task I want to work on for some time now but it's sadly always getting unprioritized. It's not super complex but requires a bit of work as the project itself doesn't build on Linux so we would need either some partial build or to retrieve the binaries built on windows to run the (non-Windows specifics) tests on Linux!

I guess such is life :D. There is never enough time for everything.

Oh right, I definitely didn't consider all cases there... I am opening a ticket for that part (see #1730).

Thank you again for fixing this.

Indeed that seems like a complex scenario... At the same time if "some\ stuff" is supposed to be the folder to copy that means this is something that will work only on Linux so there is no big issue. Maybe we could introduce some escaping, if you use the escaping then we don't normalize the path.

As I said - there might be a solution for using backslashes, etc, but I don't think it is worth the effort or if it is even possible to handle them all. As long as the unix path format (excluding backslashes) works on both you could just have a warning if there are backslashes in the path under linux and recommend using just "/" or a sentence about it in the docs or something similar. From my perspective that would be enough.