test_: services approach #6171
test_: services approach #6171
Conversation
igor-sirotin
requested review from
antdanchenko,
fbarbu15,
yevh-berdnyk and
osmaczko
Jenkins BuildsClick to see older builds (9)
|
|
@osmaczko correctly pointed out that it might be an overkill to test such simple thing with a function test. |
|
|
||
| class WalletService(Service): | ||
| def __init__(self, client: RpcClient): | ||
| super().__init__(client, "wallet") |
There was a problem hiding this comment.
LGTM, I like this approach
| @@ -80,7 +78,8 @@ def create_account_and_login(self, data_dir="/", display_name=DEFAULT_DISPLAY_NA | |||
| } | |||
| return self.api_valid_request(method, data) | |||
|
|
|||
| def restore_account_and_login(self, data_dir="/",display_name=DEFAULT_DISPLAY_NAME, user=user_1): | |||
| def restore_account_and_login(self, data_dir="/",display_name=DEFAULT_DISPLAY_NAME, user=user_1, | |||
| network_id=31337): | |||
There was a problem hiding this comment.
maybe let's add 31337 to constants, something like
DEFAULT_NETWORK_ID
There was a problem hiding this comment.
@fbarbu15 100% agree with you, It should definitely be done
But it's better do it outside of this PR not to mess up things. 31337 is duplicated in quite many places already.
| @@ -28,15 +29,16 @@ class StatusBackendTestCase: | |||
| SignalType.NODE_READY.value | |||
| ] | |||
|
|
|||
| network_id = 31337 | |||
There was a problem hiding this comment.
and then you don't need to define it here again
| assert 'error' in json_response | ||
| assert json_response['error']['message'] == expected_error | ||
|
|
||
| # Test invalid contract |
There was a problem hiding this comment.
shouldn't this be a different test? This way having Test invalid network fail would lead to Test invalid contract not beeing executed
There was a problem hiding this comment.
@fbarbu15 good point. It should probably even be possible to parametrize this test.
But what I've done instead, is I've rewritten this in status-go, so I will completely remove it from here. Just keep the services approach proposal.
status-go/services/wallet/token/balancefetcher/balance_fetcher_test.go
Lines 386 to 393 in a345c52
Turned out that this test is much simpler on status-go level with mocks. Thanks @osmaczko for heads up. Details in #6171 (comment).
igor-sirotin
force-pushed
the
fix/GetBalancesAtByChain-error
branch
from
082b392 to
c7903a8
Compare
igor-sirotin
force-pushed
the
test/failing-get_balance_at_by_chain
branch
from
47c6a28 to
e811c66
Compare
igor-sirotin
changed the base branch from
fix/GetBalancesAtByChain-error
to
develop
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #6171 +/- ##
============================================
+ Coverage 13.91% 60.93% +47.02%
============================================
Files 817 832 +15
Lines 108323 109894 +1571
============================================
+ Hits 15068 66962 +51894
+ Misses 91332 35084 -56248
- Partials 1923 7848 +5925
Flags with carried forward coverage won't be shown. Click here to find out more. |
igor-sirotin
force-pushed
the
test/failing-get_balance_at_by_chain
branch
from
e811c66 to
338df8d
Compare
Added
ServiceandWalletService(Service)classes.This is a way to group requests by service. They use given
RpcClientand wrap requests to given service name.Instance of
WalletServiceis created inStatusBackendTestCase.@antdanchenko @fbarbu15 @yevh-berdnyk let me know if you like the initiative.
Fixed linter complaining about
params=[]default argument valueAdded
network_idargument torestore_account_and_login4. Implementedtest_get_balance_errortest for #6169(this PR is targeting that PR branch)
closes #6308