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

Add node switcher #68

Merged
merged 14 commits into from Oct 26, 2021
Merged

Add node switcher #68

merged 14 commits into from Oct 26, 2021

Conversation

kchojn
Copy link
Collaborator

@kchojn kchojn commented Oct 14, 2021

  • Added node switcher - if one is unavailable, it uses the others available to connect with node
  • Changed exception Web3ConnectionException to NodeConnectionException
  • Added tests
  • Changed tests directory structure

ethtx/providers/node/base.py Outdated Show resolved Hide resolved
ethtx/providers/node/pool.py Outdated Show resolved Hide resolved
ethtx/providers/web3_provider.py Show resolved Hide resolved
tests/providers/node/base_model_test.py Outdated Show resolved Hide resolved
ethtx/providers/node/connection.py Outdated Show resolved Hide resolved
Comment on lines 13 to 15
@classmethod
def setup_class(cls):
cls.pool = NodeConnectionPool(nodes=MAINNET_CHAIN)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to create new pool for every test, just to make sure starting state for tests is always the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tests go from top to bottom, so I think it's not necessary to duplicate object creation, where the object is not changed and logic is simple.

Copy link
Contributor

Choose a reason for hiding this comment

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

In no way test should be dependent on other tests, and there is no guarantee to run in order. There are at least 2 situations when this would be an obstacle, first one is when someone tries to make a single test pass, and instead of running this single test after implementing changes in code, he's forced to run whole class of tests. Second one is parallelism, with this approach we are losing part of functionalities from plugins like xdist for pytest, in short, we can't run every test in different process, we are limited to classes per process.

tests/providers/node/pool_test.py Outdated Show resolved Hide resolved
ethtx/providers/web3_provider.py Show resolved Hide resolved
ethtx/exceptions.py Outdated Show resolved Hide resolved
Comment on lines 13 to 15
@classmethod
def setup_class(cls):
cls.pool = NodeConnectionPool(nodes=MAINNET_CHAIN)
Copy link
Contributor

Choose a reason for hiding this comment

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

In no way test should be dependent on other tests, and there is no guarantee to run in order. There are at least 2 situations when this would be an obstacle, first one is when someone tries to make a single test pass, and instead of running this single test after implementing changes in code, he's forced to run whole class of tests. Second one is parallelism, with this approach we are losing part of functionalities from plugins like xdist for pytest, in short, we can't run every test in different process, we are limited to classes per process.

@kchojn kchojn merged commit 885b236 into EthTx:master Oct 26, 2021
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.

None yet

2 participants