-
Notifications
You must be signed in to change notification settings - Fork 665
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
[resharding] Add pytest for checking RPC calls after resharding #10296
[resharding] Add pytest for checking RPC calls after resharding #10296
Conversation
Waiting for successful run https://nayduck.near.org/#/run/3306 |
Both the new tests passed |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10296 +/- ##
==========================================
+ Coverage 71.77% 71.82% +0.04%
==========================================
Files 712 712
Lines 142859 143055 +196
Branches 142859 143055 +196
==========================================
+ Hits 102540 102744 +204
+ Misses 35611 35596 -15
- Partials 4708 4715 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
nightly/pytest-sanity.txt
Outdated
pytest --timeout=120 sanity/rpc_tx_resharding.py | ||
pytest --timeout=120 sanity/rpc_tx_resharding.py --features nightly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would rename to resharding_* so that the files are close to each other in IDE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm, yeah, I had thought about this, but I had the consideration the primary test was actually closer to RPC than resharding. I can change if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed name
pytest/tests/sanity/resharding.py
Outdated
@@ -15,7 +15,7 @@ | |||
from configured_logger import logger | |||
from cluster import get_binary_protocol_version, init_cluster, load_config, spin_up_node | |||
from utils import MetricsTracker, poll_blocks | |||
from resharding_lib import append_shard_layout_config_changes, get_epoch_offset, get_genesis_num_shards, get_genesis_shard_layout_version, get_target_num_shards, get_target_shard_layout_version | |||
import resharding_lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: how about from resharding_lib import *
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally don't think it's a great idea to do import *
, it's considered as bad practice due to potential collision of namespace and difficulty in locating the import module.
In case you don't like the resharding_lib.abc
, I can change this back to from resharding_lib import abc, pqr
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I prefer having the actual code as short as possible and I don't care about long imports.
|
||
def __setup_account(self, account_id, nonce): | ||
encoded_block_hash = self.node.get_latest_block().hash_bytes | ||
account = key.Key.from_random(account_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creates two different accounts with some specific balance in shard3 and (future) shard4
assert response == transfer_response, response | ||
pass | ||
|
||
def test_rcp_tx_resharding(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: rpc
client_config_changes=client_config_changes) | ||
self.node = nodes[0] | ||
|
||
# The shard boundries are at "kkuuue2akv_1630967379.near" and "tge-lockup.sweat" for shard 4 and 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: boundaries
# The first account before and after resharding is in shard 4 | ||
# The second account after resharding is in shard 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shards are indexed from 0; it should be 3 and 4 respectively
This PR adds a basic pytest to check if the RPC calls to tx endpoint work after a resharding event.
Note that this test currently works under the assumption that the node is tracking all shards and future improvements to the tests where we may need to redirect the request to a node tracking the specific shard we are interested in, would come later.
Resolving #5047