MSBuild failure when no approval files in project

[dgurram]

Hi, I'm experiencing an odd msbuild error under certain circumstances. The error I get is:

C:\Users\builduser\.nuget\packages\approvaltests\5.0.4\build\ApprovalTests.targets (0, 0)
The expression ""%(FileName)".SubString(0, -1)" cannot be evaluated. Length cannot be less than zero. Parameter name: length

This occurs during Clean for projects that have the ApprovalTests nuget package installed, but don't contain any approval files. It doesn't happen everywhere though. On my local machine it does not happen, but it does on our TFS build server which has Visual Studio 2017 and is using msbuild v15 (same combination as my local machine). One key difference is that we perform a nuget restore before the msbuild step (which invovles a clean and then a build). The projects are .Net Framework 4.6.1 and use PackageReference style.

We have a common internal Testing nuget package which references our standard test helper libraries, such as ApprovalTests, NSubstitute and so on. This is the reason that ApprovalTests is installed on test projects that don't yet have any approval tests (but anyone may want to add approval tests at any time).

This problem can be worked around by adding an empty file like Ignore.This.approved.json to those projects. However, it would be nice not to have to do this.

This might be an msbuild quirk and may well not be an issue in .net core so probably a diminishing problem over time. However, it might be easily fixable by changing the approach taken in the ApprovalTests.targets file. Currently it does this, which as I understand collects all of the C# files corresponding to the approval files in the project into the None item:

<ItemGroup>
  <None Update="**\*.approved.*"
          DependentUpon="$(
    [System.String]::Copy('%(FileName)')
      .SubString(
        0,
        $([System.String]::Copy('%(FileName)').IndexOf('.'))
      )
  )$(ProjectExt.Replace('proj', ''))" />
</ItemGroup>

The same can be achieved in this way which avoids the Substring call which seems to be the source of the problem:

<ItemGroup>
  <ApprovalFiles Include="**\*.approved.*" />
  <ApprovalFilesTransformed Include="@(ApprovalFiles)">
    <NewFileName>$([System.String]::Copy('%(Identity)').Split('.')[0])</NewFileName>
    <NewExtension>$(ProjectExt.Replace('proj', ''))</NewExtension>
  </ApprovalFilesTransformed>
  <None Include="@(ApprovalFilesTransformed->'%(NewFileName)%(NewExtension)')" />
</ItemGroup>

What do you think?

Dinesh

[dgurram]

@isidore or @SimonCropp is anyone able to look at this? Would you like a pull request? Thanks.

[SimonCropp]

happy to consider a PR, but that snippet you shared does not actual perform the same operation. it does not handle nesting the approved files under the test file using a DependentUpon

[dgurram]

Thanks @SimonCropp I didn't think of that. I will try and improve that and get a PR together.

Some additional info when I dug into it a bit more - the code as it is first fails due to the command being split over multiple lines. The error in that case looks more like this:

C:\Users\builduser\.nuget\packages\approvaltests\5.0.4\build\ApprovalTests.targets(0,0): Error MSB4184: The expression "[System.String]::Copy('%(FileName)')
.SubString(
0,
$([System.String]::Copy('%(FileName)').IndexOf('.'))
)" cannot be evaluated.

When I manually changed this to a single line, i.e.

<ItemGroup>
  <None Update="**\*.approved.*"
          DependentUpon="$([System.String]::Copy('%(FileName)').SubString(0, $([System.String]::Copy('%(FileName)').IndexOf('.'))))$(ProjectExt.Replace('proj', ''))" />
</ItemGroup>

that's when it produced the error I reported when I opened this issue. That error can be worked around with a dummy approval file as I mentioned, e.g. Ignore.This.approved.json but the package in its current state we can't work around at all.

So independent of a PR for a better fix, do you think you could update the DependentUpon value in to a single line?

Thanks.

[SimonCropp]

how about this?

  <ItemGroup>
    <None Update="**\*.approved.*">
      <ParentExtension>$(ProjectExt.Replace('proj', ''))</ParentExtension>
      <DotIndex>$([System.String]::Copy('%(FileName)').IndexOf('.'))</DotIndex>
      <ParentFile>$([System.String]::Copy('%(FileName)').SubString(0,%(DotIndex)))</ParentFile>
      <DependentUpon>%(ParentFile)%(ParentExtension)</DependentUpon>
    </None>

[dgurram]

I can't be a 100% sure but it seemed to be Substring that caused issues, which sounds weird - that should just work but then perhaps msbuild parsing is the real issue. As you've broken it out more, it might work fine.

Since my last comment I've also got it to work for the DependentUpon use case:

  <ItemGroup>
    <None Update="**\*.approved.*"
      DependentUpon="$([System.String]::Copy('%(Identity)').Split('.')[0])$(ProjectExt.Replace('proj', ''))" />
  </ItemGroup>

It's quite similar to your original but it uses Split instead of Substring.

[SimonCropp]

ok can u try 5.1.1

[dgurram]

@SimonCropp 5.1.1 worked great. Thanks for the quick turn-around.