Skip to content

Conversation

@slang25
Copy link
Member

@slang25 slang25 commented Jan 22, 2025

Note to self, do smaller commits!

This is a few changes:

  • Remove the default placeholder text about "build your test project with full debug information to get better error messages" because:
    • No one should be using full PDBs in 2025, it's relevant to older versions of .NET Framework (event the recent versions of that support portable PDBs)
    • If the stack parsing failed, this might be due to other reasons, such as NativeAOT, we shouldn't expect users to have to go and switch this off manually for these scenarios.
    • It's not really a problem unless we say it is. If a user see an assertion message which is missing the source expression text, then they may say "Hmmm, I wonder why I don't see the source expression text, maybe I want to look into why that's not working", but otherwise it may be something they just don't know they need, let's not ruin their day.
  • Perform the stack trace parsing in a try/catch, so that users still get a meaning message if this logic fails.
  • Re-enable optimizations - This is a customer expectation when using a NuGet library, it should be built in release mode, and this should include enabling optimizations. By marking all public assertion methods as NoInline, our stack walking works. I've tested this using a release mode test app, and running assertions in a tight loop, this causes a reJIT where the ShouldBe... method gets inlined. This is a new feature in .NET 7 where methods across assembly boundaries can be inlined by the Dynamic PGO.
  • Add [EditorBrowsable(EditorBrowsableState.Never)] to extension classes, to keep them from cluttering user intellisense.
  • Add convention tests to enforce the above changes to future proof things.
  • Removed AppVeyor logic for tests.

@slang25 slang25 marked this pull request as ready for review January 22, 2025 15:26
@slang25 slang25 changed the title Relax missing actual expression Relax missing actual argument expression Jan 22, 2025
@slang25 slang25 requested a review from Copilot January 23, 2025 10:44
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 27 out of 42 changed files in this pull request and generated no comments.

Files not reviewed (15)
  • src/DocumentationExamples/DocumentationExamples.csproj: Language not supported
  • src/Shouldly/Shouldly.csproj: Language not supported
  • src/Shouldly.Tests/ConventionTests/IgnoreOnAppVeyorLinuxFact.cs: Evaluated as low risk
  • src/Shouldly/Internals/StringHelpers.cs: Evaluated as low risk
  • src/Shouldly/ShouldStaticClasses/ShouldThrowTaskAsync.cs: Evaluated as low risk
  • README.md: Evaluated as low risk
  • src/Shouldly/ShouldlyExtensionMethods/ShouldBe/DateTimeShouldBeTestExtensions.cs: Evaluated as low risk
  • src/Shouldly/ShouldStaticClasses/ShouldCompleteInExtensions.cs: Evaluated as low risk
  • src/Shouldly/ShouldStaticClasses/DynamicShould.cs: Evaluated as low risk
  • src/Shouldly/ShouldlyExtensionMethods/ShouldBe/NumericShouldBeTestExtensions.cs: Evaluated as low risk
  • src/Shouldly.Tests/ConventionTests/ApprovePublicApi.cs: Evaluated as low risk
  • src/Shouldly/ShouldStaticClasses/ShouldThrowTask.cs: Evaluated as low risk
  • src/Shouldly/ShouldStaticClasses/ShouldThrow.cs: Evaluated as low risk
  • src/DocumentationExamples/ShouldMatchApprovedExamples.cs: Evaluated as low risk
  • src/Shouldly.Tests/ConventionTests/ShouldlyConventions.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)

src/Shouldly/Internals/SourceCodeTextGetter.cs:58

  • The _determinedOriginatingFrame assignment should be handled correctly to ensure it is only set if the file exists. Consider adding a check to handle this.
_determinedOriginatingFrame = fileName != null && File.Exists(fileName);

@slang25 slang25 merged commit 695be93 into master Jan 23, 2025
5 checks passed
@slang25 slang25 deleted the relax-missing-actual-expression branch January 23, 2025 11:11
@slang25 slang25 added this to the 4.3.0 milestone Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants