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

Autofac: To add default registration for IReqnrollOutputHelper #362

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Antwane
Copy link

@Antwane Antwane commented Dec 19, 2024

🤔 What's changed?

Registered the IReqnrollOutputHelper dependency with the Autofac container so is available to use out of the box.

⚡️ What's your motivation?

It's related to this issue #357

🏷️ What kind of change is this?

  • ⚡ New feature (non-breaking change which adds new behavior)

♻️ Anything particular you want feedback on?

The change is to resolve the IReqnrollOutputHelper from TestThreadContext, I would like advise in possible side effects that it could have if any.

📋 Checklist:

  • I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Users should know about my change
  • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

traceListenerMock.Object,
attachmentHandlerMock.Object);

_testThreadContainer.RegisterInstanceAs<IReqnrollOutputHelper>(outputHelper);
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we're performing a manual registration as part of the test setup, the test doesn't appear to actually exercise the production code. Can this test be rewritten or an alternative test provided to validate the production code:

  • Runs without an issue
  • Makes it possible to resolve the output helper

Copy link
Author

@Antwane Antwane Jan 6, 2025

Choose a reason for hiding this comment

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

Hi there, thanks for your suggestion, I tried implementing what you proposed but I struggled understanding what is requested and how to approach it. I removed the registration of the helper class and use the default registration from the dependency provider but I don't know if that is what was intended. The part I don't understand is why my change is not exercised in production code. As far as I can see I can debug the code and the registration is definitely hit by the unit test, the reason I had to inject manually is because the container must have it before is registered on the container but that happens on a different part of the application that is not entirely related to Autofac, the current tests seem to inject manually what is needed and then resolve it through Autofac so I aimed to do same, if a different approach is required please let me know.

@gasparnagy gasparnagy changed the title To add default registration for IReqnrollOutputHelper Autofac: To add default registration for IReqnrollOutputHelper Jan 6, 2025
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.

2 participants