Skip to content
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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EjaYF
Copy link

@EjaYF EjaYF commented Aug 2, 2022

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

  • 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.

@SabotageAndi
Copy link
Contributor

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
Copy link
Author

EjaYF commented Aug 2, 2022

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants