Skip to content

Conversation

@jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell marked this pull request as ready for review April 3, 2025 05:53
@jamescrosswell jamescrosswell requested a review from aritchie as a code owner April 3, 2025 05:53
@jamescrosswell jamescrosswell requested a review from Flash0ver April 4, 2025 03:54
@Flash0ver
Copy link
Member

Flash0ver commented Apr 7, 2025

question: For which types "should" we add unit tests?

  • DisabledHub.cs
  • HubAdapter.cs
  • Hub.cs
  • SentrySdk.cs

We should then also add missing tests for the equivalent CaptureEvent that takes an Action<Scope> configureScope.

We could also extract this comment to a new issue, and complete the test coverage separately, should this bloat or conflate the scope of this PR. (I'm happy to submit that as my first changeset 😉)

@jamescrosswell jamescrosswell changed the title Added CaptureFeedback overload with configureScope parameter #4035 Added CaptureFeedback overload with configureScope parameter Apr 7, 2025
@jamescrosswell
Copy link
Collaborator Author

jamescrosswell commented Apr 8, 2025

We could also extract this comment to a new issue, and complete the test coverage separately, should this bloat or conflate the scope of this PR. (I'm happy to submit that as my first changeset 😉)

I'd rather have the unit tests included in the PR that they relate to. Otherwise it becomes really easy for us to adopt a "build now test later never" approach.

Normally I'd say since I wrote the code, I should be adding the unit tests (and reviewers like yourself should be insisting I do so). But if you want to make some first commits, you're welcome to add some unit tests to this PR and we can share the glory.

For which types "should" we add unit tests?

DisabledHub is a bunch of no-ops... I think it's a bit of a waste of time writing tests for that. There are some, but I don't really see the point of them and they haven't been maintained.

You'll see examples of tests for all of the others though:

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.

Add CaptureFeedback overload accepting a scope parameter

4 participants