Made scenario and feature context items more thread-safe

SpecFlowOSS / SpecFlow

#1 .NET BDD Framework. SpecFlow automates your testing & works with your existing code. Find Bugs before they happen. Behavior Driven Development helps developers, testers, and business representatives to get a better understanding of their collaboration

https://www.specflow.org/

Other

2.24k stars 754 forks source link

Made scenario and feature context items more thread-safe #2629

Open EjaYF opened 2 years ago

EjaYF commented 2 years ago

The Dictionary behind the ScenarioContext and FeatureContext is not thread-safe. Different kind of errors can occur if tests are executed with parallel access to the ScenarioContext/FeatureContext Dictionary. I've added a sample unit test which fails in case of using a default Dictionary in stead of a ConcurrentDictionary to demonstrate this. I think some open issues can be related to this.

Types of changes

[x] 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:

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

SabotageAndi commented 2 years ago

Hmm, I am not sure this fixes anything at the end and hides issues in your automation code.

The reason is, that every Scenario gets its own ScenarioContext anyway. So two Scenarios will never access the same instance of the scenario context. It would only be a problem if you start your own threads in a scenario. Not sure this is a good idea either.

And if you have multiple scenarios that save stuff in the FeatureContext and they overwrite each other, you have some issues in your automation code anyway. The ConcurrentDirectory doesn't fix them.

Could you elaborate on why this change fixes stuff for you? Or did I miss something here?

@gasparnagy what are your thoughts about this?

EjaYF commented 2 years ago

In our case we store the outcome of a step in the ScenarioContext for which we test the validity in another step by accessing this outcome as stored in the ScenarioContext. The outcome of a step is mostly generated by a service implementation that heavily uses asynchronous calls and thus requires this Dictionary to be thread safe, especially when the outcome is stored in multiple key/value pairs.

It would only be a problem if you start your own threads in a scenario. Not sure this is a good idea either.

The purpose is to call and test business logic from within a Scenario. Whether or not that business logic uses threading should not be an issue and should be supported by SpecFlow in any case in my humble opinion.

SabotageAndi commented 2 years ago

The purpose is to call and test business logic from within a Scenario. Whether or not that business logic uses threading should not be an issue and should be supported by SpecFlow in any case in my humble opinion.

You are absolutely right. But shouldn't the business logic call at the end be completely done, before you continue with the code in your step definition?

Or do you pass the Dictionary to your Business Logic?

I am not against changing this to ConcurrentDictionary. I just want to understand what is getting fixed with that change.