Skip to content

Update per-test config to be unique per-test in spec_configured_state_test#2765

Merged
hwwhww merged 4 commits intoethereum:ex-ante-testsfrom
ralexstokes:fix-spec-config-concurrency-issue
Dec 7, 2021
Merged

Update per-test config to be unique per-test in spec_configured_state_test#2765
hwwhww merged 4 commits intoethereum:ex-ante-testsfrom
ralexstokes:fix-spec-config-concurrency-issue

Conversation

@ralexstokes
Copy link
Member

Alternative fix for the issue addressed in #2755.

As I understand it, the issue there was that using the spec_configured_state_test decorator was not "thread-safe" so that any unsynchronized access could cause issues. These issues were seen in #2752 when executing the tests with pytest-xdist which parallelizes tests across multiple cores.

@hwwhww found the issue and suggested a fix w/ lazy loading of the modules in the pyspec.

Rather than go around making multiple copies of various modules, this PR takes a different approach to tests using the spec_configured_state_test decorator: simply make a separate copy of the Spec object.

As long as instances of Spec don't get too big or resource-intensive then we should be able to easily specialize a given Spec for each test that uses this decorator by simply making a copy and making the necessary edits.

As far as I can tell, there is no reason to "restore" the spec after mutation as it is discarded after the result from the decorated test is obtained.

@ralexstokes ralexstokes changed the base branch from dev to ex-ante-tests December 7, 2021 16:10
@ralexstokes
Copy link
Member Author

update: the Spec is a python module where we cannot simply just copy it. thinking of other solutions....

@ralexstokes
Copy link
Member Author

after chatting w/ @hwwhww came to the current solution using importlib to load a fresh copy of the spec per test that needs to customize it, but skipping the lazy loading in #2755 and also only loading a duplicate in the specific tests that need customization, not every test

@ralexstokes
Copy link
Member Author

there is some strange failure w/ a test (test_config_override) that seems to pass on dev

but ignoring that for now, i'd suggest we do something like this PR instead of #2755 to fix #2752 (and anything else that uses that decorator) as it provides a distinct, scoped instance of the spec to each test that wants to mutate it.

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

Big thanks to @ralexstokes

Compared to #2755, it gets rid of the needless imports largely. Well done!

@hwwhww hwwhww merged commit c3c24fb into ethereum:ex-ante-tests Dec 7, 2021
@ralexstokes ralexstokes deleted the fix-spec-config-concurrency-issue branch December 7, 2021 18:28
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