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

librification: Remove global state from _stbt #333

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

Conversation

wmanley
Copy link
Contributor

@wmanley wmanley commented Feb 24, 2016

The get/set_config mechanism is a way of storing state in configuration files. This state is global, meaning that you couldn't have different devices under test configured separately in the same process.

This commit fixes that issue by pulling a config object into the DeviceUnderTest class. This is a fairly blunt approach but it works for now. Perhaps in the future the specific keys that the DeviceUnderTest class needs can be directly injected.

I'm not 100% sold on this change yet, but I've created this PR as a place to discuss. This is obviously related to #297, although I'm not planning on ticking all the rest of the items from there off immediately.

The `get/set_config` mechanism is a way of storing state in configuration
files.  This state is global, meaning that you couldn't have different
devices under test configured separately in the same process.

This commit fixes that issue by pulling a config object into the
`DeviceUnderTest` class.  This is a fairly blunt approach but it works for
now.  Perhaps in the future the specific keys that the `DeviceUnderTest`
class needs can be directly injected.
@@ -587,10 +598,12 @@ def new_device_under_test_from_config(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_device_under_test needs to pass config into DeviceUnderTest.

@drothlis
Copy link
Contributor

I think the config isn't the most critical bit of global state to get rid of. I'd be perfectly happy with a stbt that could be imported even if half the functions call get_config internally which reads from a global config object. I'd rather spend my time reviewing the other things on the #297 list, but if you think this is important then by all means we can proceed.

I imagine you'd also want to get rid of the _stbt.config._config global object? This pull request so far injects the _stbt.config module into the places that use get_config, but it's still a module with global state.

Whatever we do, the following must still use the match parameters from the config file:

$ PYTHONPATH=/usr/lib/stbt:$PYTHONPATH ipython
>>> import stbt
>>> stbt.match(...)

I think that's still the case with this pull request in its current form, but it's a bit hard to reason about. I think what's confusing me is the way we pass around the _stbt.config module as an object. Or maybe it's just inevitable that using dependency injection properly means you have to trace through a lot of code to figure out where things are coming from.

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

Successfully merging this pull request may close these issues.

None yet

2 participants