-
Notifications
You must be signed in to change notification settings - Fork 108
Add docco warning against resolving ScenarioContext here #1004
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
Conversation
A cautionary tale - if you attempt to resolve the `ScenarioContext` from this event, then it won't have been created/initialised by the `ContextManager` yet. That means that the DI container attempts to create it during resolution. If that happens, you'll encounter an object container exception, because the container in-turn tries to resolve a `RuleInfo` instance, which also has yet to be added to the container. Because `RuleInfo` has ctor params which include primitives (strings), this throws because the container doesn't permit the resolution of primitives (for good reason). I suspect that this came about because of the changes made here: * This PR: reqnroll#454 * Particularly this comment: reqnroll#454 (review) I think it's most likely that it was never intended for a plugin to attempt to resolve `ScenarioContext` from this position; the fact that it had worked before the addition of `RuleInfo` is likely a coincidence. So, I think that this docco is a useful addition, to warn future plugin-authors away from doing something naughty, which could take a while to diagnose.
The cause of this and the remediation is documented on this pull request for Reqnroll: reqnroll/Reqnroll#1004 In short: I mustn't attempt to resolve ScenarioContext from that extension point. Instead resolve ScenarioInfo, which is all I ever needed anyway. The improved logic works with both Reqnroll 2.0.0+ and the legacy SpecFlow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds XML documentation to the CustomizeScenarioDependencies event in the RuntimePluginEvents class. The documentation warns plugin authors not to resolve ScenarioContext from this event, as it will cause an ObjectContainerException because the scenario context hasn't been initialized at that point. The documentation suggests using ScenarioInfo instead.
Changes:
- Added XML documentation comment with summary and remarks to the
CustomizeScenarioDependenciesevent
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
gasparnagy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx!
🤔 What's changed?
Add a C# documentation comment for the
CustomizeScenarioDependenciesevent hook (for plugin authors). This warns them off attempting to resolve aScenarioContext, as doing so will result in a crash error which is difficult to diagnose.⚡️ What's your motivation?
A cautionary tale - if you attempt to resolve the
ScenarioContextfrom this event, then it won't have been created/initialised by theContextManageryet. That means that the DI container attempts to create it during resolution.If that happens, you'll encounter an object container exception, because the container in-turn tries to resolve a
RuleInfoinstance, which also has yet to be added to the container. BecauseRuleInfohas ctor params which include primitives (strings), this throws because the container doesn't permit the resolution of primitives (for good reason).FYI, resolving scenario context from this location had worked in old SpecFlow and worked in Reqnroll 2.x. I suspect that this came about because of the changes made here:
RuleInfotoScenarioContext#454RuleInfotoScenarioContext#454 (review)CustomizeScenarioDependenciesis invoked, it seems that the context manager initialization hasn't happened yetI think it's most likely that it was never intended for a plugin to attempt to resolve
ScenarioContextfrom this position; the fact that it had worked before the addition ofRuleInfois likely a coincidence. That's why I wouldn't describe this as a bug or regression, even though "something that used to work doesn't anymore". In essence the already-documented breaking change in 3.0.0 has a slightly wider impact than previously documented. That's why I think that this docco is a useful addition, to warn future plugin-authors away from doing something bad, which could take a while to diagnose.In my specific case, I found out that I was being stupid and resolving
ScenarioContextfrom this event, when all I ever wanted wasScenarioInfo. So, I've fixed my own plugin code by resolving the info type and not the context type.🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
Are my presumptions in the Motivations section correct? If not then maybe this needs more than just a docco tweak?