Check untyped defs in all integration tests except #5129

karlb · 2019-10-21T13:18:27Z

the ones in integration.network.

PR review check list

Quality check list that cannot be automatically verified.

Safety
- The changes respect the necessary conditions for safety (https://raiden-network-specification.readthedocs.io/en/latest/smart_contracts.html#protocol-values-constraints)
Code quality
- Error conditions are handled
- Exceptions are propagated to the correct parent greenlet
- Exceptions are correctly classified as recoverable or unrecoverable
Compatibility
- State changes are forward compatible
- Transport messages are backwards and forward compatible
Commits
- Have good messages
- Squashed unecessary commits
If it's a bug fix:
- Regression test for the bug
  - Properly covers the bug
  - If an integration test is used, it could not be written as a unit test
Documentation
- A new CLI flag was introduced, is there documentation that explains usage?
Specs
- If this is a protocol change, are the specs updated accordingly? If so, please link PR from the specs repo.
Is it a user facing feature/bug fix?
- Changelog entry has been added

the ones in `integration.network`.

palango

Nice, we're getting close.

Fine to merge when the tests pass.

raiden/tests/integration/test_pythonapi.py

LefterisJP

Looks good. Just asked for some changes. But tests fail for probably unrelated error

LefterisJP · 2019-10-21T15:18:25Z

raiden/tests/integration/api/test_pythonapi.py

-        code = app0.raiden.rpc_client.web3.eth.getCode(
-            to_checksum_address(token_network_address), block_number
-        )
+        code = app0.raiden.rpc_client.web3.eth.getCode(token_network_address, block_number)


nitpick use kwargs

LefterisJP · 2019-10-21T15:26:03Z

raiden/tests/integration/api/test_restapi.py

-        app0.raiden.rpc_client,
-        contract_manager=contract_manager,
-        constructor_arguments=(token_amount, 2, "raiden", "Rd"),
+    new_token_address = TokenAddress(


Why not create a helper function for this deployment which also returns the proper type?

LefterisJP · 2019-10-21T15:27:55Z

raiden/tests/integration/test_balance_proof_check.py

        timeout=network_wait * number_of_nodes,
    )
    # stop app1 - the test uses token_network_contract now
    app1.stop()
    token_network_contract = app1.raiden.proxy_manager.token_network(token_network_address)
    empty_balance_proof = BalanceProof(
        channel_identifier=channel_identifier,
-        token_network_address=to_checksum_address(token_network_contract.address),
+        token_network_address=TokenNetworkAddress(token_network_contract.address),


Nice find. I had to go check the code to make sure that you did not make a mistake here. How come the test was passing before?

Probably because they still got converted into the desired format everywhere where it is actually used (pack_data, etc.). I didn't check in detail.

LefterisJP · 2019-10-21T15:30:27Z

raiden/tests/integration/test_pythonapi.py

-        deploy_client=app1.raiden.rpc_client,
-        contract_manager=contract_manager,
-        constructor_arguments=(token_amount, 2, "raiden", "Rd"),
+    token_address = TokenAddress(


same as above for the helper function here and in other parts in this file

We need one in multiple tests, so this is a nice way to make the tests more concise.

codecov · 2019-10-23T14:46:54Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@b3f5905). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop    #5129   +/-   ##
==========================================
  Coverage           ?   80.63%           
==========================================
  Files              ?      120           
  Lines              ?    14776           
  Branches           ?     2296           
==========================================
  Hits               ?    11914           
  Misses             ?     2178           
  Partials           ?      684

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3f5905...50ab14b. Read the comment docs.

Check untyped defs in all integration tests except

29445e1

the ones in `integration.network`.

karlb marked this pull request as ready for review October 21, 2019 13:28

auto-assign bot requested a review from LefterisJP October 21, 2019 13:28

palango approved these changes Oct 21, 2019

View reviewed changes

raiden/tests/integration/test_pythonapi.py Show resolved Hide resolved

LefterisJP reviewed Oct 21, 2019

View reviewed changes

karlb added 2 commits October 21, 2019 18:46

Use kwargs for getCode

6e0c6da

Introduce unregistered_token fixture

50ab14b

We need one in multiple tests, so this is a nice way to make the tests more concise.

palango merged commit 9b356b0 into raiden-network:develop Oct 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check untyped defs in all integration tests except #5129

Check untyped defs in all integration tests except #5129

karlb commented Oct 21, 2019

palango left a comment

LefterisJP left a comment

LefterisJP Oct 21, 2019

LefterisJP Oct 21, 2019

LefterisJP Oct 21, 2019

karlb Oct 21, 2019

LefterisJP Oct 21, 2019

codecov bot commented Oct 23, 2019

Check untyped defs in all integration tests except #5129

Check untyped defs in all integration tests except #5129

Conversation

karlb commented Oct 21, 2019

PR review check list

palango left a comment

Choose a reason for hiding this comment

LefterisJP left a comment

Choose a reason for hiding this comment

LefterisJP Oct 21, 2019

Choose a reason for hiding this comment

LefterisJP Oct 21, 2019

Choose a reason for hiding this comment

LefterisJP Oct 21, 2019

Choose a reason for hiding this comment

karlb Oct 21, 2019

Choose a reason for hiding this comment

LefterisJP Oct 21, 2019

Choose a reason for hiding this comment

codecov bot commented Oct 23, 2019

Codecov Report