autofac / Autofac

An addictive .NET IoC container
https://autofac.org
MIT License
4.44k stars 835 forks source link

Add none generic service middleware registration methods in Autofac #1417

Open idiotsky opened 3 months ago

idiotsky commented 3 months ago

These changes introduce new methods to register service middleware in the Autofac library. Also, a new test has been added to Autofac.Specification.Test to ensure that the middleware is getting invoked correctly when a service is registered. These methods allow for more precise control of the pipeline during the resolve request process.

alistairjevans commented 3 months ago

Hi @idiotsky, can you please elaborate on the purpose of these new methods? What issue are you trying to address?

Typically we'd raise an issue to discuss a feature request or bug before we open a PR.

alistairjevans commented 3 months ago

Ah, ok, this is for #1416.

idiotsky commented 3 months ago

does my change broken the build, on my side all the test passed? looks the unit test failed not related to my code

tillig commented 3 months ago

I mean, this isn't a question I can answer off the top of my head, it's just what we see:

Unclear if it's due to a problem in the test or something in this PR or what. But that's what I see - the same thing as you.

I kicked off the build again to see if it consistently fails.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 78.51%. Comparing base (4547d16) to head (2cddfd3).

Files Patch % Lines
...Autofac/ServiceMiddlewareRegistrationExtensions.cs 66.66% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1417 +/- ## =========================================== - Coverage 78.52% 78.51% -0.02% =========================================== Files 200 200 Lines 5713 5719 +6 Branches 1168 1168 =========================================== + Hits 4486 4490 +4 - Misses 714 716 +2 Partials 513 513 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tillig commented 3 months ago

Build seems to be passing now, but it appears the coverage is unhappy.

I haven't gotten around to reviewing this yet (but it looks like it's reasonable); I did see there was one unit test added to the RegistrationOnlyIfTests which seemed an odd place to add tests for a middleware registration method. After doing some searching, it appears we actually don't have specific tests about RegisterServiceMiddleware, so that seems like a shortcoming.

Here's what I'd say:

That should take care of the coverage issue and give us a place where we can put additional tests as needed.