-
Notifications
You must be signed in to change notification settings - Fork 245
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
Integration tests: transferTx_value, transferTx_from #5802
base: develop
Are you sure you want to change the base?
Conversation
Jenkins BuildsClick to see older builds (18)
|
@@ -9,16 +9,16 @@ class RpcTestCase: | |||
def setup_method(self): | |||
self.network_id = 31337 | |||
|
|||
def verify_is_valid_json_rpc_response(self, response, _id=None): | |||
def verify_is_valid_json_rpc_response(self, response, _id=None, _error=False): |
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.
let's create a separate method, verify_is_jscon_rpc_error
@@ -29,6 +29,19 @@ def verify_is_valid_json_rpc_response(self, response, _id=None): | |||
raise AssertionError(f"no id in response {response.json()}") | |||
return response | |||
|
|||
def validate_error_code_content(self, response, error_code: int, error_content): |
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.
I would appreciate to keep it as assert error code and assert error content in the test itself, meanwhile https://github.com/status-im/status-go/pull/5802/files#r1745360470 will be called first to ensure that we have error
instead of result in response
class TransactionTestCase(RpcTestCase): | ||
|
||
def wallet_create_multi_transaction(self): | ||
@staticmethod | ||
def _update_multitx_method(params, updates): |
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.
it is advanced, on my opinion for better readability would be great to pass each parameter explicitly to wallet_create_multi_transaction
@antdanchenko |
raise AssertionError( | ||
f"no required text in message: {actual_error_text} instead of expected: {expected_error_text}" | ||
) | ||
with open(f"{option.base_dir}/schemas/wallet_createMultiTransaction/transferTx_error", "r") as schema: |
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.
sorry, didn't notice on first review, we can use https://github.com/status-im/status-go/blob/develop/integration-tests/tests/test_cases.py#L45 here, I will also update other tests
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.
can you elaborate please?
response = self.wallet_create_multi_transaction(**changed_values) | ||
self.verify_is_json_rpc_error(response) | ||
actual_error_code, actual_error_text = response.json()['error']['code'], response.json()['error']['message'] | ||
if expected_error_code != actual_error_code: |
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.
I would prefer to have native asserts, e.g.
assert actual_error_code != expected_error_code, f"got code: {actual_error_code} instead of expected: {expected_error_code}"
as result error message will be
AssertionError: got code: -32000 instead of expected: 20000
also saves some space :)
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.
done
raise AssertionError( | ||
f"got code: {actual_error_code} instead of expected: {expected_error_code}" | ||
) | ||
if expected_error_text not in actual_error_text: |
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.
we can do same as https://github.com/status-im/status-go/pull/5802/files#r1756993745 but with assert not in
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.
done
14b8222
to
cc2e9ef
Compare
as they are committed to this branch. Should I just store them in main dir? UPDATE: turned out (who would have thought?) that Linux is case-sensitive for paths and Mac is not. |
b3bfa4f
to
3a7a623
Compare
Added first parametrized tests from checking errors in create_multi_transaction method (note, that I just put a random
str
tofrom
to show that it is easily expandable)In future will expand with more values