reqnroll
Introducing the `REQNROLL_DRY_RUN=true` env var, to use the DryRunBindingInvoker when executing tests
🤔 What's changed?
Updates Reqnroll to look for an environment variable, REQNROLL_DRY_RUN, which causes it to optionally register the DryRunBindingInvoker on top of the default one.
For example, if you execute:
dotnet test -e REQNROLL_DRY_RUN=true .\MyTestProject.csproj
The test suite will no longer actually invoke the step handlers. However, if you've configured "Inconclusive" test results to be interpreted as a failure, this will effectively allow you to verify that steps in all .feature files are referencing a defined step handler. (For example during PRs.)
⚡️ What's your motivation?
This enables users to run CI verification on their test suite to ensure all .feature files are referencing a defined step handler. Right now the only way to verify this is at test execution runtime, which can be prohibitive for users that have implemented expensive tests using this framework.
🏷️ What kind of change is this?
- :zap: New feature (non-breaking change which adds new behaviour)
♻️ Anything particular you want feedback on?
I couldn't find a particularly obvious place for this check to go. So I ended up rationalizing to myself that it could live in ContainerBuilder, following a similar convention that the BindingProviderService follows.
I also opted to have the environment variable check more-or-less hardcoded in the business logic rather than trying to elevate it to an object model like ReqnrollConfiguration.cs. (Though the check does use the IEnvironmentWrapper abstraction.) It felt like designing this feature use the "normal" configuration system wasn't the most accessible way to utilize this functionality, as opposed to an environment variable, because that would require somehow updating your config JSON only for CI.
That said, I don't feel totally confident in my chosen implementation. I welcome other contributor's opinions on a better place for this to live!
I'd also appreciate suggestions on where this feature should live in the docs, which are yet to be updated.
📋 Checklist:
- [x] I've changed the behaviour of the code
- [x] I have added/updated tests to cover my changes.
- [x] My change requires a change to the documentation.
- [x] I have updated the documentation accordingly.
- [x] Users should know about my change
- [x] 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.
In addition to comments I've left on specific lines of code, I suggest we find a spot in the documentation to describe this. Your PR description provides a great use-case that could/should be described there.
@clrudolphi This has been done 👍🏻
@DrEsteban Sorry to do this to you, but got clarification from Gaspar about the naming convention of environment variables. Please revert it back to a single underscore.
Based on further discussion, I think I'm going to adjust strategies again and update the default binding provider to have this behavior.
Based on further discussion, I think I'm going to adjust strategies again and update the default binding provider to have this behavior.
Yes, please do that and sorry for dragging you back and forth.
@gasparnagy, @clrudolphi, @Code-Grump, thanks so much for your insightful feedback and discussion! I think the last push represents the final desired state of this PR, so I'm going to move it out of draft phase.
Of course, please continue to add any additional feedback you may have for this latest refactor.
I have no problem with embedding the dry-run logic directly into the
BindingInvokerand the implementation looks good.I would like to discuss the merits of resolving the environment variable within the invoker versus moving that to where Configuration is resolved. Perhaps this setting should be part of the ReqnrollConfiguration object?
@clrudolphi I considered that, but didn't think the UX was particularly nice given the use cases.
Having it be part of the configuration would require some magic in a CI pipeline to swap the reqnroll.json before building and testing, or doing some sort of in-place file update. Neither of which seemed as nice as simply setting dotnet test -e ....
I suppose there's a possible middle ground here, where we add it to the configuration object but also allow overriding via environment variable--likely having the env var take precedence if set. (I don't think that's how existing configuration settings work, so that'd be establishing a new behavior.) Overall I believe users would find it annoying to have this setting be JSON only. Thoughts?
I didn't make myself explicit enough. I did not mean to imply that the setting should be included in the reqnroll.json but that the value of the environment variable should be resolved when the Reqnroll configuration is read and that the value of the environment variable be added to the ReqnrollConfiguration object.
Chris
From: Steven Ryland @.>
Sent: Sunday, June 1, 2025 4:10:01 AM
To: reqnroll/Reqnroll @.>
Cc: Chris Rudolphi @.>; Mention @.>
Subject: Re: [reqnroll/Reqnroll] Introducing the REQNROLL_DRY_RUN=true env var, to use the DryRunBindingInvoker when executing tests (PR #614)
[https://avatars.githubusercontent.com/u/1495037?s=20&v=4]DrEsteban left a comment (reqnroll/Reqnroll#614)https://github.com/reqnroll/Reqnroll/pull/614#issuecomment-2926869134
I have no problem with embedding the dry-run logic directly into the BindingInvoker and the implementation looks good.
I would like to discuss the merits of resolving the environment variable within the invoker versus moving that to where Configuration is resolved. Perhaps this setting should be part of the ReqnrollConfiguration object?
@clrudolphihttps://github.com/clrudolphi I considered that, but didn't think the UX was particularly nice given the use cases.
Having it be part of the configuration would require some magic in a CI pipeline to swap the reqnroll.json before building and testing, or doing some sort of in-place file update. Neither of which seemed as nice as simply setting dotnet test -e ....
I suppose there's a possible middle ground here, where we add it to the configuration object but also allow overriding via environment variable. (Likely having the env var take precedence if set.) Thoughts?
— Reply to this email directly, view it on GitHubhttps://github.com/reqnroll/Reqnroll/pull/614#issuecomment-2926869134, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAM7YMTTJSHOZWUI6KXXGP33BK7OTAVCNFSM6AAAAAB5RUS2FSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSMRWHA3DSMJTGQ. You are receiving this because you were mentioned.Message ID: @.***>
Another completely different approach that occurred to me would be to replace the registration of the BindingInvoker (in the DefaultDependencies bc class) with a factory that would return either the regular invoker or the dry run invoker based upon the value of the environment variable.
I didn't make myself explicit enough. I did not mean to imply that the setting should be included in the reqnroll.json but that the value of the environment variable should be resolved when the Reqnroll configuration is read and that the value of the environment variable be added to the ReqnrollConfiguration object.
@DrEsteban @clrudolphi I suggest doing that in two steps. Let's just keep the env variable reading in the invoker right now and lets refactor the env variable to config stuff as a second step (separate PR). We have other env var usages as well (opt out telemetry), so it would make sense to have a generic common solution for that.
Another completely different approach that occurred to me would be to replace the registration of the BindingInvoker (in the DefaultDependencies bc class) with a factory that would return either the regular invoker or the dry run invoker based upon the value of the environment variable.
This would maybe work as well indeed, but let's stick to the built-in approach for now. We can refactor is later anyway.
Great points from both of you.
I also like the idea of having a follow-up PR that refactors all env var-based configuration into ReqnrollConfiguration so all configurable options are contained vs sprinkled around the codebase. For context, I went with the embedded approach exactly because that seemed to be the existing convention for other components that consume env vars. But it seems that usage of IEnvironmentWrapper isn't even consistent, so a PR focusing on that sounds like a good call.
I definitely don't mind the idea of using a factory for this instead of basically re-implementing DryRunBindingInvoker logic into the default BindingInvoker, however, I do wonder if baking it into BindingInvoker is a better UX. If we want to present "dry run" as a first-class feature of Reqnroll - I'm imagining some complicated use-case where a user plugin optionally overrides the IAsyncBindingInvoker implementation, but falls back to re-registering BindingInvoker in some cases because they believe that's Reqnroll's "default". Which is true, but then it might be confusing why the dry run feature no longer works for them, because they override the default factory lambda. I guess it could be argued that if a user is already this far into the weeds it's reasonable to expect them to handle these concerns for themselves 😆 But I do think there's something to be said for having first-class features implemented into the default classes themselves rather than implementing them via wrapper factories. At least with the way the plugin model currently works.
Anyway....
TLDR: @gasparnagy, @clrudolphi, unless I'm missing something, I think that means this PR is ready to go in as-is. Any additional thoughts or feedback you have at this stage?