Add non allocating ConfigureScope overload

[KnapSac]

Fixes #4228.

[KnapSac]

I changed this test slightly because I couldn't get it to work with the new overload. It seems to me like this still functionally tests the same thing as before, although I'm not even sure the test implementation was correct.

The test used to first invoke the ConfigureScope callback and then assert that the scope is locked, or am I misunderstanding that?

[jamescrosswell]

I think the previous test was fine. It basically invoked the configurescope callback on the local var scope when this line executed (since the hub at this point is a substitute):

sentry-dotnet/src/Sentry/HubExtensions.cs

Line 178 in 34d8490

public static void LockScope(this IHub hub) => hub.ConfigureScope(c => c.Locked = true);

Then it checked to ensure that scope.Locked == true afterwards.

I don't think the new test is validating anything since the test code itself is trying to set scopeLocked to true and then checking to see if it's true aftewards... it's not actually testing any code in the SDK.

Note that hub.PushAndLockScope() is actually calling an extension method (so you can't mock this with NSubstitute). Ultimately what's being tested here is this line of code:

sentry-dotnet/src/Sentry/HubExtensions.cs

Line 181 in 6863c2e

public static void UnlockScope(this IHub hub) => hub.ConfigureScope(static s => s.Locked = false);

Did HubSubstituteExtensions.SubstituteConfigureScope not work here? It looks like you used this successfully everywhere else...

[KnapSac]

Yeah, you're right. Those substitute methods take some getting used to ;). I'll take a look and try to get the old test working.

[KnapSac]

I'm not sure why I can't remove the first substitute in this method, its Then callback doesn't seem to be called, but if I remove it the test no longer passes...

[jamescrosswell]

I assume you're referring to this one:

sentry-dotnet/test/Sentry.AspNetCore.Tests/SentryMiddlewareTests.cs

Lines 161 to 169 in dc5c853

	_fixture.Hub
	.When(h => h.ConfigureScope(Arg.Any<Action<Scope>>()))
	.Do(Callback
	.First(c => c.ArgAt<Action<Scope>>(0).Invoke(scope))
	.Then(_ =>
	{
	Assert.True(scope.Locked);
	verified = true;
	}));

That would ordinarily get called by HubExtensions.LockScope here:

sentry-dotnet/src/Sentry/HubExtensions.cs

Line 175 in ee4ce7d

public static void LockScope(this IHub hub) => hub.ConfigureScope(static s => s.Locked = true);

Since the test uses a substitute for the Hub, that behaviour has to be imitated by the substitute.

It's definitely a bit tricky to walk down all the paths mentally and then setup the appropriate substitutes.

In any event, this test looks all good now 👍🏻

[codecov]

Codecov Report

Attention: Patch coverage is 86.53846% with 7 lines in your changes missing coverage. Please review.

Project coverage is 72.86%. Comparing base (495e742) to head (dc5c853).
Report is 637 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Sentry/Internal/Hub.cs	53.84%	6 Missing ⚠️
src/Sentry/Extensibility/HubAdapter.cs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4244      +/-   ##
==========================================
- Coverage   75.73%   72.86%   -2.87%     
==========================================
  Files         357      458     +101     
  Lines       13466    16662    +3196     
  Branches     2671     3320     +649     
==========================================
+ Hits        10198    12141    +1943     
- Misses       2593     3672    +1079     
- Partials      675      849     +174     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jamescrosswell]

Fancy 😜

[jamescrosswell]

Generally looking really good - just need to to get that one test working and I think we're good to go.

[bruno-garcia]

Just some minor suggestions but otherwise LGTM! Thanks!

[Flash0ver]

test: Should we have a test for StartSpan in HubExtensionsTests.cs

To be fair, these method was recently moved and made public, where we should have added a test for this, but missed it.
Now, that this method calls an internal method, I'm wondering if this is adding more "worth" of adding a test for this extension method.

@KnapSac, only if you feel like adding a test for this method.
To be fair, though, it should have been added in a preceding PR.