Service.GetValueRetrieverFor : Collection was modified, ...

[blemasle]

SpecFlow Version

3.9.22

Which test runner are you using?

xUnit

Test Runner Version Number

2.4.1

.NET Implementation

.NET 5.0

Project Format of the SpecFlow project

Sdk-style project format

.feature.cs files are generated using

SpecFlow.Tools.MsBuild.Generation NuGet package

Test Execution Method

Visual Studio Test Explorer

SpecFlow Section in app.config or content of specflow.json

No response

Issue Description

Hi,

Service.GetValueRetrieverFor enumerate the collection as it goes in the foreach loop because ServiceComponentList does not use an intermediary collection.

Any modification of ValueRetrievers during the enumeration by another thread will lead to InvalidOperationException: Collection was modified; enumeration operation may not execute..

When adding retrievers within a method marked with [BeforeTestRun], we do encounter the issue occasionaly on our CI and very rarely during tests execution in VS.

It might be easily fixed by enumerating the collection (with a ToArray() or similar) before returning the enumerator on https://github.com/SpecFlowOSS/SpecFlow/blob/master/TechTalk.SpecFlow/Assist/ServiceComponentList.cs#L13

This would also mean the ToList() on line 47 is no longer needed.

Steps to Reproduce

I sadly cannot find a deterministic way to reproduce this as it is heavily dependent of the test runner and probably need a whole bunch of tests and scenario to reproduce.

We just added a retriever registration in a binding marked with [BeforeTestRun].

[Binding]
public static class GlobalSteps
{
    [BeforeTestRun]
    public static void BeforeTestRun()
    {
        Service.Instance.ValueRetrievers.Register<MyRetriever>();	
    }
}

Link to Repro Project

No response

[SabotageAndi]

How do you get a BeforeTestRun hook executed multiple times with xUnit? These hooks should only be called once also when you run multiple threads.

[blemasle]

🤔 Indeed. I only guessed the error originates from multiple execution of that hooks since it was the only place ValueRetrievers were modified.

Recently the problem has gotten worse as we faced the need to add & remove retrievers based on scenario tags. I didn't want to go into specifics so I kept that out of the initial description and stick with the original issue we were facing before. Apologies 🙂

The current use and real problem then comes from the retrievers we add/remove based on some tags with BeforeScenario and AfterScenario. For instance, we use something similar to https://github.com/corvus-dotnet/Corvus.Testing/blob/main/Solutions/Corvus.Testing.SpecFlow/Corvus/Testing/SpecFlow/ChildObjectBindings.cs

[SabotageAndi]

Changing the retrievers per tag per scenario doesn't work in a parallel execution scenario, as the memory store behind it is static (https://github.com/SpecFlowOSS/SpecFlow/blob/master/TechTalk.SpecFlow/Assist/Service.cs#L11).

[blemasle]

I found out the hard way :( I'm curious, is there any conceptual reason behind that static memory store rather being stored within the ScenarioContext ?

[SabotageAndi]

We never thought that somebody would need to adjust the registrations per scenario. Also it is quite hard to have a static API and access to the per-scenario container.

I am happy to review any PR that improves this part of the SpecFlow code base.

[ajeckmans]

@SabotageAndi Are you still happy to get such a PR? We have a situation where it would be really handy to have the custom value retriever get a dependency from the scenario context. This would mean a per-scenario instance of the value retriever is needed.

I'd think that if we just make it that the TechTalk.SpecFlow.Assist.Service is injected, we would be able to resolve that and add extension methods on it akin to the TableExtensionMethods etc. Instead of writing table.CreateSet<>(), you'd write service.CreateSet<>(table). It would mean anyone using beforescenario is able to change the list of value retrievers and inject per-scenario dependencies. I think this approach would also mean going the route of ScenarioContext.Current and deprecate the old way, but still let it work for the time being.

And to give a use case: we have human readable names for technical ids. The human readable names are transformed into the technical ids by way of value retrievers and stepargumenttransformers. The human readable names, along with the generated ids are registered into a per-scenario context object. We have a step implementation that reads that context and persists it in the per-scenario database for later use in the scenario. The technical ids are complex enough (there is a relation between ids as the objects are persisted in a graph) that we wanted to hide the details in the feature files and the step implementations as good as possible.

[SabotageAndi]

@ajeckmans Sorry, but I can't help you or say anything if a PR will get accepted. I don't work for Tricentis and SpecFlow anymore.