Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add none generic service middleware registration methods in Autofac #1417

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

idiotsky
Copy link

@idiotsky idiotsky commented Apr 3, 2024

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.

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
Copy link
Member

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
Copy link
Member

Ah, ok, this is for #1416.

@idiotsky
Copy link
Author

idiotsky commented Apr 3, 2024

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

@tillig
Copy link
Member

tillig commented Apr 5, 2024

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

  • The build has been stable for all other PRs.
  • The build failed on this PR.

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.

Copy link

codecov bot commented Apr 5, 2024

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 ⚠️
Additional details and impacted files
@@             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              

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

@tillig
Copy link
Member

tillig commented Apr 5, 2024

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:

  • Add a class Autofac.Specification.Test.Registration.ServiceMiddlewareTests.
  • Put your tests for your new registrations in there. They should be named like RegisterServiceMiddleware_ScenarioNameHere since that's the method they're testing.
  • Add at least one test for each of the new overloads so the overloads get exercised. The test should verify that the registered middleware ran as expected.

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

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.

None yet

3 participants