Improve context storage to share data through whole run #396
Conversation
…znaranjo/toolium into feat/improve_storage
…znaranjo/toolium into feat/improve_storage
There was a problem hiding this comment.
Redesigned again with Chainmap, this time making sure it works, just to give more information:
ChainMap unifies, or links, the 3 context dictionaries into one
Considering we have a mapchain( Dict1, Dict2, Dict3) where dicts are storage, feature and run:
- If i get a value with context.storage["key"] the first key found in the 3 dictionaries will be retrieved, accesing them by order from left to right. So if we have a duplicated key in storate and run storage, it will get the storage one. -> To avoid this, the programmer should be responsible of where the key is added, but we handle it if you use the store_in_context function.
- If i set a value into context.storage, it will be stored in the first dictionary always, so it will always be stored in context.storage so this leaves the context.storage to work as the previous behavior as desired and expected.
- context.storage and run_storage are linked in before all, so instead of a chainmap, any change you do to storage is stored in run, which is an expected behaviour since the only existant context in before all is the run_storage.
| def store_key_in_storage(context, key, value): | ||
| """ | ||
| Store values in context.storage, context.feature_storage or context.run_storage, | ||
| using [key], [FEATURE:key] OR [RUN:key] from steps. | ||
| context.storage is also updated with given key,value | ||
| By default, values are stored in context.storage. | ||
|
|
||
| :param key: key to store the value in proper storage | ||
| :param value: value to store in key | ||
| :param context: behave context | ||
| :return: | ||
| """ | ||
| clean_key = re.sub(r'[\[\]]', '', key) | ||
| if ":" in clean_key: | ||
| context_type = clean_key.split(":")[0] | ||
| context_key = clean_key.split(":")[1] | ||
| acccepted_context_types = ["FEATURE", "RUN"] | ||
| assert context_type in acccepted_context_types, (f"Invalid key: {context_key}. " | ||
| f"Accepted keys: {acccepted_context_types}") | ||
| if context_type == "RUN": | ||
| context.run_storage[context_key] = value | ||
| elif context_type == "FEATURE": | ||
| context.feature_storage[context_key] = value | ||
| # If dynamic env is not initialized linked or key exists in context.storage, the value is updated in it | ||
| if hasattr(context.storage, context_key) or not isinstance(context.storage, collections.ChainMap): | ||
| context.storage[context_key] = value | ||
| else: | ||
| context.storage[clean_key] = value |
There was a problem hiding this comment.
Info: This function should always be used by the programmer if the expected behaviour is to use feature and run storage. Otherwise it's the programmer responsability to handle and avoid duplicated keys, etc if they use directly the context.storage[key]=x and context.feature_storage[key]=x
We also kept the FEATURE: and RUN: tags as it gives more flexibility.
legnadev
requested review from
josemiguel-rodrigueznaranjo,
rgonalo and
SamuelAranda
and removed request for
josemiguel-rodrigueznaranjo
|
Code Climate has analyzed commit 71f8905 and detected 0 issues on this pull request. View more on Code Climate. |
There was a problem hiding this comment.
The new logic based on ChainMaps to search for storage keys is OK.
The new function to store values in one place or another depending on their prefix does not seem like a good programming practice to me, especially in the context of BDD, and I would remove it.
| # Dictionary to store information between features | ||
| context.feature_storage = dict() | ||
| context.storage = collections.ChainMap(dict(), context.feature_storage, context.run_storage) |
There was a problem hiding this comment.
| context.storage = collections.ChainMap(dict(), context.feature_storage, context.run_storage) | |
| context.storage = collections.ChainMap(context.feature_storage, context.run_storage) |
There was a problem hiding this comment.
Both ways seems to work correctly. So choose your preference.
There was a problem hiding this comment.
As requested, please. I think it makes more sense and is more consistent with the new logic added to before_all and before_scenario.
Improved the managing of storages:
run_storageto store information during the whole test executionstore_key_in_storageto manage it