Installing Shouldly silently disables optimization and changes other properties

[loop-evgeny]

I've run into a similar issue before (#723) but this wasted hours of my time again: performance tests suddenly started failing for no explicable reason, when the code being tested did not change. The problem turned out to be Shouldly's .props file disabling optimization:

<?xml version="1.0" encoding="utf-8"?>
<Project>
  <PropertyGroup>
    <DebugSymbols>true</DebugSymbols>
    <Optimize>false</Optimize>
    <DebugType>embedded</DebugType>
    <Deterministic>false</Deterministic>
    <DeterministicSourcePaths>false</DeterministicSourcePaths>
  </PropertyGroup>
</Project>

This can be verified at runtime by checking for the DebuggableAttribute (and I'm adding a unit test for it now). With Shouldly referenced by my test project optimization is disabled. It also still changes DebugType and, apparently disables Deterministic builds and source paths.

Guys, wasting your users time in this way is really not cool. The project looks promising, but this sort of stuff has me very close to giving up on using it. You may have your reasons for settings those properties, but please do not set build properties without a huge, impossible to ignore warning to the user somewhere. If you cannot provide a huge warning then don't do it at all and let the user do it manually if that's truly needed for Shouldly to work. For my usage, it seems to work just fine without those. (Tested this by just referencing the DLL directly, without the NuGet package.)

[jnm2]

I hear you. Huge, impossible-to-ignore warnings for the 99% case are a non-starter I think, but maybe it would be an acceptable tradeoff to put a smaller amount friction in the 99% path and not have Shouldly work out of the box, but fail with a test message that says "Set this csproj property: ...". I think even this would diminish the user experience significantly and provide a poor impression of Shouldly for first-time users, even leading to many more people having the experience of their time being wasted.

If there was an idea that wouldn't involve nags or friction in the vast majority of cases, we'd jump at it, but I'm honestly not sure there can be a solution that works well everywhere. Without such an option, I'm not comfortable making a decision on this without the total agreement of @josephwoodward and @slang25.

[jnm2]

if that's truly needed for Shouldly to work.

A central draw of Shouldly is that it generates great error messages. It only generates great error messages if it can locate source files based on the stack trace when a test fails. We don't currently know of a way to obtain the original source file without setting these properties, but we can also try again.

[loop-evgeny]

"Set this csproj property: ...". I think even this would diminish the user experience significantly and provide a poor impression of Shouldly for first-time users

I don't think it's really that hard to set the properties and most users would already have symbols enabled, at least in the Debug build where they're typically running failing tests. Certainly it would increase friction to some extent - but it's nowhere near as bad as having your performance tests failing for no explicable reason and wasting hours trying to figure this out and then having to manually reference DLLs.

I think, at minimum, the build property changes should be split into a separate NuGet package. The error message can tell the user "You can manually set these properties OR if you're OK with having us change the build in ways that might break other stuff, reference this other NuGet package". There also needs to be documentation on why each of these is needed - I still don't understand the reason for most of them.

DebugSymbols=true - OK, that's an obvious one, but I already had that enabled.
Optimize=false - I'm guessing due to inlining variables sometimes, but I haven't run into any problems with Optimize=true
DebugType=embedded - why embedded specifically? This itself caused me problems earlier (#723) and DebugType=full seems to work just fine.
Deterministic=false - no idea why this is needed
DeterministicSourcePaths=false - no idea why this is needed, but it's false by default

Loosely related: but when I first tried Shouldly in an F# test that failed it told me that it cannot find the source files. When I removed the NuGet reference and added the DLL directly (i.e. avoided all the .props) it then generated the proper error message, so in this case the .props actually seemed to break it! I couldn't repro that unfortunately, so didn't include that in the bug report, but it further strengthens my view that this is all project-specific and, unfortunately, not a matter of "just include this file and everything will magically work".

[slang25]

I agree that what Shouldly is doing here is too intrusive. From digging around and from memory, the initial problem was a bug where the stack walking within Shouldly wasn't happening reliably, however this shouldn't require a change to the consuming projects from what I understand.

I think we wanted/needed the portable PDBs of Shouldly to be loaded by the consuming runtime (which we still do, as 'full' is legacy), however they were not loaded by default in .NET 4.7.1 and lower. I had a PR to address this, however again it's a little intrusive (but less intrusive that then eventual fix IMO):
#548

Shoudly should be able to work with a typical .NET Core / .NET 5 project without special targets IMO. Even with a Release build, provided symbols are present (which they are by default).

[daveaglick]

FWIW, I just got sidetracked by this too and if it weren't for this issue I might have spent many hours on it. In my case, I use Shouldly in a testing support library that's intended to be used within other unit test projects, but doesn't contain any tests of it's own. My support library is published to NuGet and all of a sudden, after an upgrade from Shouldly 3.x to 4.x, my publishing scripts stopped working due to missing symbol files.

For anyone also stumbling on this behavior and wanting to exclude the .props import, you can add the following to your PackageReference:

<ExcludeAssets>build</ExcludeAssets>

[nathan-c]

I also ran into an issue trying to debug a .NET6 unit test project (while upgrading from .NET5) where Rider couldn't load symbols and so never hit my breakpoints. I eventually worked out that embedded doesn't appear to be a valid option anymore.

Manually selecting Portable fixed my issue but we have many unit test projects using Shouldly meaning I need to change all the csproj files to include <DebugType>portable</DebugType>. I use linux and am not sure if this is a linux specific thing or if this affects windows too.

[iBrotNano]

In my case only the deterministic settings are a problematic. It prevents referencing projects to be built deterministically, too. Maybe if this setting is not necessary you could remove it from build.props and put it in your Directory.Build.props. In build.ps1 i could see, that you have tried to build a deterministic build on the buildserver. As it is now you hinder yourself to do so.

I do not fully understand the problem, so I can not judge if the described configuration is a solution.

https://github.com/shouldly/shouldly/blob/master/src/Shouldly/build.props
https://github.com/shouldly/shouldly/blob/master/src/Directory.Build.props
https://github.com/shouldly/shouldly/blob/master/build.ps1

[PureKrome]

@ Shouldy team - would love some feedback on this issue, especially after what @iBrotNano and @daveaglick have both suggested, above. 🙏🏻

[slang25]

I would accept a PR with a fix, we shouldn't be setting these sort of properties on consumers behalfs.

[PureKrome]

🚀Awesome news @slang25 ! Cheers! 🍻🥂

There you go, readers .. any chance someone with a bigger brain 🧠 than I, might be able to help? 🏥

[iBrotNano]

... you could remove it from build.props and put it in your Directory.Build.props. In build.ps1 i could see, that you have tried to build a deterministic build on the buildserver. As it is now you hinder yourself to do so.

Ok. I have forked and built the project by myself and the resulting nuget packed by the CI.yml build is deterministic. The script build.ps1 works. But the uploaded nuget on https://www.nuget.org/packages/Shouldly/4.1.0 ist not deterministic. This was the one i inspected.

[jaredpar]

Does anyone know why the determinism settings are turned off? All determinism does in the compiler is ensure that

1. A deterministic MVID is produced
2. A deterministic timestamp is produced
3. A deterministic PDB ID is produced

That's it compared to /deterministic=false. Not sure what benefit is coming from disabling these, particularly for getting fluent assertions. Am I missing some benefit here? It's definitely going to cause issues for consumer who rely on, or at least expect, determinism in their builds.

[slang25]

@jaredpar We've merged in a change to revert this and to handle deterministic builds differently: #883 (your opinion and review is welcome)

Shouldly has provided a CallerArgumentExpression like feature that goes back to the .NET Framework days where the stacktrace is walked to find the call site source, this requires mapping mapping document paths in debug symbols back to source files on disk.

I will look at adding CallerArgumentExpression which eventually will remove the need for this behaviour going forwards, but for now we'll get a release out that reverts these intrusive property changes.

[jaredpar]

@slang25 sorry been a bit behind. Changes look good!