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

Use eth-tester. #815

Merged
merged 26 commits into from
May 17, 2018
Merged

Use eth-tester. #815

merged 26 commits into from
May 17, 2018

Conversation

pipermerriam
Copy link
Contributor

@pipermerriam pipermerriam commented May 10, 2018

Let's do this!

if get_key(value) in values_to_remove:
continue
self.queue.put_nowait(value)
eth_tester.utils.filters.Filter.remove = Filter_remove
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like a good case for using the monkeypatch fixture that pytest provides assuming you are not aware of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

wasn't aware thanks ;)

return chain.contract(source_code, language="vyper", *args, **kwargs)
return get_contract
def assert_tx_failed(tester):
def assert_tx_failed(function_to_test, exception=TransactionFailed):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These would be pretty easy to implement using contextlib.contextmanager if you wanted to niceness of pytest.raises(...)

@contextlib.contextmanager
def tx_fails(exception=...):
    try:
        yield
    except exception:
        pass
    else:
        raise AssertionError("Did not raise")

def get_logs(w3):
def get_logs(tx_hash, c, event_name):
tx_receipt = w3.eth.getTransactionReceipt(tx_hash)
logs = getattr(c._classic_contract.events, event_name)().processReceipt(tx_receipt)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

latest web3 now supports c._classic_contract.events[event_name]

# Check bidding time is 5 days
assert auction_tester.c.auction_end() == auction_tester.s.head_state.timestamp + 432000
assert auction_contract.auction_end() == tester.backend.chain.header.timestamp + EXPIRY - 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you want to be future proof, I'd recommend changing this to not reach into the backend and to instead use tester.get_block_by_number['latest']['timestamp']. In fact, now that I've seen this, I'm inclined to rename the location of backend to tester._backend so that it's clear that it's a private API.

}
tx = w3.eth.sendTransaction(deploy_transaction)
address = w3.eth.getTransactionReceipt(tx)['contractAddress']
contract = w3.eth.contract(address, abi=abi, bytecode=bytecode, ContractFactoryClass=ConciseContract)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see you are using ConciseContract copiously. My first inclination was to push you away from it, but I think that's mostly my personal preference speaking.

Was the choice to use this happenstance? Intentional over the more explicit call semantics for the default Contract class?

Copy link
Contributor

@jacqueswww jacqueswww May 10, 2018

Choose a reason for hiding this comment

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

@pipermerriam the biggest reason for the vyper use case is to be able to call functions directly on the contract object.
For vyper easily 95% or more of the tests, you want to call a function and get it's output. That's why opted to just keep get_contract the same as before.



def test_assert_refund(t, get_contract_with_gas_estimation):
def test_assert_refund(w3, tester, get_contract_with_gas_estimation, assert_tx_failed):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curious of the choice to use both w3 and tester here. w3.eth.getBalance(...) should allow you to avoid direct tester interactions which is mostly just academic, but it does reduce how closely your API is tied to the eth-tester API (and the w3 API is likely to be more stable).

Copy link
Contributor

@jacqueswww jacqueswww May 11, 2018

Choose a reason for hiding this comment

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

I agree, for tests direct tester acces should be minimised, and w3 and fixtures maximised will change accordingly ;)

assert c.foo(startgas=50000) > 25000
assert c.foo(call={"gas": 50000}) < 50000
assert c.foo(call={"gas": 50000}) > 25000

print('Passed gas test')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

total nitpick on my part, but converting these print statements to use logging allows you to get the same output in your terminal while still allowing that output to be easily silenced, or even tuned to only show a subset by logger or by log level.

def test_block_number(get_contract_with_gas_estimation, chain):
chain.mine(1)
def test_block_number(get_contract_with_gas_estimation, tester):
tester.mine_blocks(1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just in case you aren't aware, w3.testing.mine(n) can be used if you want to stick with web3 APIs

Copy link
Contributor

@jacqueswww jacqueswww May 10, 2018

Choose a reason for hiding this comment

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

neat, I didn't know. Yes I think having tests use web3 as much as possible is the way to go.


assert c.bar() == utils.sha3("inp")
assert '0x' + c.bar().hex() == sha3(b"inp").hex()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduces readability on the assert statement but you could drop the hex overhead and do the direct assertion.



@pytest.fixture
def sha3():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd recommend renaming this to keccak. (which makes me realize we should deprecate Web3.sha3 and rename it to keccak as well.

@carver carver mentioned this pull request May 10, 2018
22 tasks
w3.toInt(x[64:]), # v
w3.toInt(x[:32]), # r
w3.toInt(x[32:64]) # s
) for x in map(lambda z: w3.toBytes(hexstr=z[2:]), raw_sigs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The [2:] is no longer necessary here. Same with all other calls with a hexstr=thing[2:]

# Check to make sure the signatures are valid
assert '0x' + utils.encode_hex(utils.sha3(utils.ecrecover_to_pub(h2, sigs[0][0], sigs[0][1], sigs[0][2]))[12:]) == accounts[0]
assert '0x' + utils.encode_hex(utils.sha3(utils.ecrecover_to_pub(h2, sigs[1][0], sigs[1][1], sigs[1][2]))[12:]) == accounts[1]
assert Account.recoverHash(h2, sigs[0]).lower() == accounts[0]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want, you can use eth_utils.is_same_address which handles equality for addresses across checksum, normal hex, and bytes representations.

@@ -1,73 +1,106 @@
import pytest

from eth_keys import KeyAPI
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can also use from eth_keys import keys and remove anywhere that you are doing keys = KeysAPI()

@fubuloubu
Copy link
Member

This PR proves the need for an opinionated testing library for python, something you can just import and know that there is always one preferred way of doing something for testing.

@jacqueswww jacqueswww changed the title For Jacques Use eth-tester. May 15, 2018
@jacqueswww jacqueswww force-pushed the eth_tester branch 2 times, most recently from 4694891 to c4c8f0f Compare May 16, 2018 15:05
@fubuloubu
Copy link
Member

Woo!

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.

3 participants