Preliminary work on adding a table helpers class that can be resolved…

[ajeckmans]

In the current table helpers extensions upon first call all ValueRetrievers are cached in a static list. This effectively freezes them in time and disallows any interaction with the scenario itself. I've found that there are a few scenario's where this would be useful. For instance, when translating from a scenario identifier to a at run-time determined technical identifier (so in the step code).

Goal of this PR is to provide a POC moving away from the table helpers extensions (table.CreateInstance<T>()) and to a from the DI resolvable instance of a new class, effectively making it possible for ValueRetrievers to be scoped to the running scenario.

This is as part of the discussion #16

Types of changes

• Bug fix (non-breaking change which fixes an issue).
• New feature (non-breaking change which adds functionality).
• Breaking change (fix or feature that would cause existing functionality to not work as expected).
• Performance improvement
• Refactoring (so no functional change)
• Other (docs, build config, etc)

Checklist:

• I've added tests for my code. (most of the time mandatory)
• I have added an entry to the changelog. (mandatory)
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

I've definitely gone with the quick and dirty approach just so we can see if this is the way we want to be going about this and also to further determine what needs to be done.

[ajeckmans]

Obviously there is still a lot to be done, but I did not want to put in the extra effort without a bit more confirmation on the approach :)

Stuff I feel like needs to be addressed:

• Check if there are enough tests surrounding scoping of DI services to guarantee the ValueRetrievers are retrieved per-scenario
• Copy over at least all of the old tests to the new approach
• Create some more compatibility tests so we can ensure the old and new way are working the same for at least the know scenario's.

Aside from checking all of the other boxes in the checklist.

[gasparnagy]

@ajeckmans thx! I think the general direction is good in my opinion.

Also it is clear how this helps for the dynamic / context specific conversion cases.

To understand the impact better (and figure out the rollout strategy), could you add an example of a usual (non dynamic) example here to the PR description, how the table helpers were used before and after (you can use the one from the docs as "before").

And one more question (I could not fully review the code yet): could we automatically use the [StepArgumentTransformation] conversions for the table conversions as well? I know that has slightly different structure, but I had to duplicate those conversions quite a few times in projects, and this is a typical question I get in courses.

[ajeckmans]

@gasparnagy I wanted to keep the scope change to a minimum for this PR as this is already a large enough change. Basically whenever this would be merged people could (and most likely will) get obsolete warnings all over the place. I want to make it as easy as possible to move over to the new approach. Which means that I've left the function signature mostly intact and that I want to ensure that there are no functionally breaking changes happening.
Any other change I'd be happy to work on as well, but after this has gone through and users have had some time to adjust their code bases.

As for the change itself. The goal is to go from this:

public class Steps {
    [Given(@"Given I entered the following data into the new account form:")]
    public void GivenIEnteredTheFollowingDataIntoTheNewAccountForm(DataTable table)
    {
        var account = table.CreateInstance<Account>();
    }
}

to this:

public class Steps {
    private readonly TableHelpers _tableHelpers;

    public Steps(TableHelpers tableHelpers) {
        _tableHelpers = tableHelpers;
    }

    [Given(@"Given I entered the following data into the new account form:")]
    public void GivenIEnteredTheFollowingDataIntoTheNewAccountForm(DataTable table)
    {
        var account = _tableHelpers .CreateInstance<Account>(table);
    }
}

Naming is work in progress as well :)

And on the value retriever registering side you can make this example do as expected in parallel execution scenario's, when using the table helper methods. This is currently broken, because the value retriever is fixed in time during the first scenario execution.

[Binding]
internal sealed class ValueRetrieverHooks
{
    [BeforeScenario]
    public static void BeforeScenario(ScenarioContext context)
    {
        var dependency = context.ScenarioContainer.Resolve<T>();
        Service.Instance.ValueRetrievers.Register(new ScenarioDependentValueRetriever(dependency));
    }
}

[gasparnagy]

@ajeckmans Makes sense. About [StepArgumentTransformation], my question was more about the feasibility and your feelings and not necessarily whether this is going to be included in this PR & change or not.

[ajeckmans]

It is feasible I think to resolve the StepArgumentTransformation and then use those to create for instance a set from a table, however.. What happens if the StepArgumentTransformation itself also uses the TableHelpers to create an instance. You could go into an infinite loop in that case, I think.

Better I think is to really look at the system of going from a table to its final object representation. As I understand it:

• the StepArgumentTransformation is a hook that gives you full flexibility to manually do the transformation from input to object. Whether that input is a Table or string or any other object.
• The value retrievers are there to support the conversion of a table to its final type using the table helpers. They do not work standalone and only operate on a single entry in the table, never the entire table nor any other input.

The scoping is different and as such I wonder where the request to use the StepArgumentTransformation during the table helpers functionality comes from. I can see a reason for every ValueRetriever to also automatically be a StepArgumentTransformation (unless a user-defined StepArgumentTransformation for the same conversion is specified) and to go away with some of the duplication between a StepArgumentTransformation and the ValueRetrieves. But for StepArgumentTransformation only those that operate on a string would be eligible to participate in the TableHelpers code.

I actually sometimes inject the custom ValueRetriever into a StepArgumentTransformation to work around the code duplication.

[gasparnagy]

It is feasible I think to resolve the StepArgumentTransformation and then use those to create for instance a set from a table.

But it would be inconsistent with the "CompareToSet" then. Do you see any chance to somehow reuse the StepArgumentTransformations for comparison as well?

What happens if the StepArgumentTransformation itself also uses the TableHelpers to create an instance. You could go into an infinite loop in that case, I think.

That problem we anyway have unfortunately and has to be solved independently, see #71.

But for StepArgumentTransformation only those that operate on a string would be eligible to participate in the TableHelpers code.

That is a good observation!

I actually sometimes inject the custom ValueRetriever into a StepArgumentTransformation to work around the code duplication.

That is right. Actually we have multiple options:

1. Automatically reuse (or opt-out) StepArgumentTransformation as ValueRetriever (only string ones of course)
2. Automatically reuse (or opt-out) ValueRetriever as StepArgumentTransformation (just the sake of completeness)
3. Do not do any connection between these, but show examples in docs how you can reuse them in the other
4. Make the reuse opt-in style: marking a particular StepArgumentTransformation as to be used as ValueRetriever (we could also do global opt-in as well, but that is quite destructive setting and therefore hard to manage).

I suggest we should think about what would be the ideal choice (ignore the backwards compatibility issues) and then we can see if that can be somehow passed to the existing behavior.

To answer the question myself, in general I would probably like to see option 1, because the StepArgumentTransformation is the primary way to define values using business language (e.g. use "yes" / "no" for a decision) and I would like to use the same consistency in data tables as well. On the other hand, if we can only use this for CreateSet and not for CompareToSet then maybe this is too confusing. In that case, probably I would go for option 3.

[ajeckmans]

To determine what to do on that aspect we would have to make clear all scenarios and which are the ones we want to support the best. I think it is a worthwhile discussion, so we can also describe better in the documentation when to use a StepArgumentTransformation and when to use the ValueRetriever and Create** methods. Howver, I think this is best left discussed outside of this PR and in a separate discussion.

[gasparnagy]

@ajeckmans you are right. The only reason why I would like to discuss this now (here or in the discussion), because the rollout strategy of the new feature depends on how we want to use it long term.

The two main strategy that I am hesitating between are the following:

1. We make the new way of table conversions as close as possible to the old one (except the static nature, of course) and try to move everyone quickly to the new style (because it is similar behavior).
2. Make a more deeper redesign and give more time to the users to switch.

I also have problems with the CompareToSet method name and the step argument transformation support is also appealing, so I would normally be for the deeper redesign. But I am not fully set.

[ajeckmans]

Right :)

Well I would welcome a redesign that would somehow consolidate the StepArgumentTransformation, the ValueRetriever and the CreateSet/CreateTable fluff. I think that would be entirely possible for almost all of the use cases I've come across. However, I still feel that is better left for a different time and PR.

It would be awesome if we could add some kind of metrics on the methods though. I wonder whether the functions with the additional configs are really used all that often. Personally I've only come across code that resembles https://ronaldbosma.github.io/blog/2022/10/29/transform-specflow-table-column/ And getting some more data on how the current features are actually used might help us make a more informed decision.

For now I think I'll go ahead with the more one on one approach and maintain almost perfect compatibility with the existing code. If we feel down the road (before or after merging this PR) we still want to redesign we will at least have a solid base to start working from and I'll have gained more knowledge on how this part of the code works.

[gasparnagy]

@ajeckmans OK, I get your point and have been thinking on it a lot now.

In this case, however, my suggestion would be to delay marking the existing API with the obsolete attribute, because

• if we will have further structural refactoring, another migration will be needed from the users anyway
• in this form, for the most of the users (who don't customize the retrievers or do that statically) the new way will not provide a direct benefit, so judging the efforts of migration is harder.

So basically we could see this as a new feature that allows using the good old assist methods in a more dynamic manner. The behavior and the API is the same, but you can configure it per-scenario now.

I know that it is not ideal, but for most of the users (I see that in my trainings) the dependency injection is not trivial. Maybe if we will support property injection, the migration path is going to be easier anyway, so we can reconsider deprecating the old API even without having the "big redesign".

We can also consider to implement metrics to learn more about what is most commonly used. (But since only a smaller fraction of the SpecFlow users have migrated so far, the numbers might not be very representative yet. But later they will be.)

What do you think?

[ajeckmans]

Ok, I would like to add something in the documentation that points people to use the DI version though.

[gasparnagy]

@ajeckmans Absolutely. In the docs we should push this!

[gasparnagy]

@ajeckmans Do you plan to work on this PR this week? We could include it to v2 in that case.

[ajeckmans]

I might have some time for this, but I cannot guarantee it.

In any case, taking the latest discussion into account I think this change won't be a breaking one anymore and as such should be able to release with any future minor release.

[gasparnagy]

In any case, taking the latest discussion into account I think this change won't be a breaking one anymore and as such should be able to release with any future minor release.

Yes. That is correct. So we are not in a hurry with this.