Skip to content

Conversation

@wassim-k
Copy link
Contributor

@wassim-k wassim-k commented Oct 4, 2025

Fixes #40

I've created unit tests to replicate the issue and they all pass including integration tests.

var streamPipelineBehaviorType = typeof(IStreamPipelineBehavior<,>);
var syncNotificationHandlerType = typeof(INotificationHandler<>);

var otherHandlerTypes = new HashSet<Type>()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is not related to the fix, but I thought we can avoid constructing the handler types array multiple times.

@codecov
Copy link

codecov bot commented Oct 4, 2025

Codecov Report

❌ Patch coverage is 99.19355% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/DispatchR/Configuration/ServiceRegistrator.cs 99.10% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@hasanxdev hasanxdev self-assigned this Oct 4, 2025
@wassim-k
Copy link
Contributor Author

wassim-k commented Oct 4, 2025

@hasanxdev Can you please provide more info on where I should be placing the tests?
I can't see NotificationHandlerTests under UnitTests
Also should I define MultiNotificationHandler, MultiRequestHandler, TestNotification1...3 and TestRequest1...3 all in TestCommon? or keep them encapsulated within the unit test?

@hasanxdev
Copy link
Owner

@wassim-k Please place the main test that includes the Fact for notifications in this file:
NotificationTests.cs

For the handlers, put them inside this file:
RequestHandlerTests.cs

If you need to create a new file, please move it into the DispatchR.TestCommon.
That applies to your files MultipleInterfacesTests and MultiRequestHandler:
PR Link

@wassim-k wassim-k force-pushed the fix/multi-handler-registrator branch from bcaf5ea to 87c2b4b Compare October 4, 2025 12:40
@wassim-k
Copy link
Contributor Author

wassim-k commented Oct 4, 2025

Tests are updated, and I can confirm 100% coverage of code changed in this PR.

@wassim-k
Copy link
Contributor Author

wassim-k commented Oct 6, 2025

Hi @hasanxdev, codecov seems to also take testing code into account, should that be excluded from the report?

@hasanxdev
Copy link
Owner

Hi @wassim-k ,
Please see if you can somehow resolve this issue, either by covering it with tests or by adjusting the way the tests are written.

Since this library is still quite new, it’s already hard for people to trust it. A 100% test coverage isn’t necessarily what makes developers adopt a library, but it’s definitely one of the key factors they consider.

If we don’t address this, all the effort we’ve put into building this library might get overlooked, because no one has the time to dig into what exactly isn’t covered, they’ll just see that the test coverage is low.

Thank you so much, and I apologize for the delayed response.

@wassim-k
Copy link
Contributor Author

wassim-k commented Oct 6, 2025

I understand, but code coverage is for library code not test code, can you configure it to exclude test projects/directory?
Otherwise, it might degrade the quality of the tests themselves when we have to, for example, invoke handlers unnecessary just to hit the code coverage.
Or is there a reason you want test code to also be 100% covered?

@hasanxdev
Copy link
Owner

Looks good bro, I’m doing the final review of your code.
99% coverage is awesome! If everything checks out, I’ll go ahead and merge it.

@hasanxdev
Copy link
Owner

Thank you for helping in the development of this library.

@hasanxdev hasanxdev merged commit ed65b23 into hasanxdev:main Oct 6, 2025
2 checks passed
@wassim-k wassim-k deleted the fix/multi-handler-registrator branch October 6, 2025 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot register multiple INotificationHandlers or IRequestHandlers on the same class

2 participants