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

Extend filter tests with dynamic types and longer block ranges #997

Merged
merged 12 commits into from
Aug 16, 2018

Conversation

dylanjw
Copy link
Contributor

@dylanjw dylanjw commented Aug 9, 2018

What was wrong?

Related to Issue #836

How was it fixed?

Adding to the suite of filter tests to show cases that are currently failing. I am separating this work from #953, by marking tests with xfail.

Test cases to cover

  • Argument filters of dynamic types
  • Argument filters of fixed types
  • Argument filters of array types
  • Topic filters of dynamic types
  • Topic filters of fixed types
  • Topic filters of array types
  • Block filters over 50+ blocks
  • Log filters over 50+ blocks
  • Transaction filters over 50+ blocks
  • Transaction filters match transactions in unmined "pending" block
  • Log filters matching a few events against many contracts, many emitted logs, many blocks.
  • Contract with invalid return data
  • EDIT:
  • Argument and topic filters with arrays of dynamic types

Cute Animal Picture

image

@dylanjw dylanjw changed the title Extend log filter tests Extend filter tests with dynamic types and longer block ranges Aug 13, 2018
@dylanjw
Copy link
Contributor Author

dylanjw commented Aug 14, 2018

Issues found so far:

  • Filtering dynamic type event arguments do not yield results. (fixed in Introduce a better filter argument api with EventFilterBuilder #953)
  • Block filter only returns partial results when a lot of blocks have been mined (~50).
  • Pending transaction filter returns partial results when eth-tester auto-mining has been turned off.
  • Getting an error deploying and transacting with contracts on circleci (works in locally run tests.)

@dylanjw
Copy link
Contributor Author

dylanjw commented Aug 14, 2018

Im getting errors deploying a contract with 2d arrays on eth-tester. I will work on adding that test in a different PR.

This is ready for review. @carver @pipermerriam

Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

Loving all the tests and hypothesis!

This is the only part that I was concerned about:

The regex based data filters do not work with dynamic sized types

I want to make sure it's not a silent bug where we are incapable of returning the events, but the user has no indication of that.

assert log_entries[0]['transactionHash'] == txn_hashes[0]


@pytest.mark.xfail(reason="The regex based data filters do not work with dynamic sized types")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What kind of failure do we get? Do we just not get the events we were looking for? It seems like we should explicitly raise an exception if we're incapable of generating the filter accurately. Then the test could be a pytest.raises() on that exception.

Copy link
Contributor Author

@dylanjw dylanjw Aug 16, 2018

Choose a reason for hiding this comment

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

It does not raise an exception, it just doesnt find results. However, these xfails are temporary. I was going to get this merged and then rebase #953. The xfail can be removed in #953. This could have been a part of #953, but Im trying to break it up into smaller PRs.

)

SEED = 50505050505
random.seed(SEED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this left in from some debugging?

expected_match_counter = 0

tx_padder = pad_with_transactions(web3)
tx_padder.send(None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're not sending anything, maybe next(tx_padder) is clearer here.

_to = accounts[random.randint(0, len(accounts) - 1)]
value = 50 + tx_count
w3.eth.sendTransaction({'from': _from, 'to': _to, 'value': value})
yield tx_hash
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not seeing the benefit of this being a generator. It seems it could be a function you call when you want to pad, like:

def pad_with_transactions(w3):
        accounts = w3.eth.accounts
        for tx_count in range(random.randint(0, 10)):
            _from = accounts[random.randint(0, len(accounts) - 1)]
            _to = accounts[random.randint(0, len(accounts) - 1)]
            value = 50 + tx_count
            w3.eth.sendTransaction({'from': _from, 'to': _to, 'value': value})

Then later, instead of invoking the generator and calling next(), you call pad_with_transactions(w3) again.

_to = accounts[random.randint(0, len(accounts) - 1)]
value = 50
tx_hash = w3.eth.sendTransaction({'from': _from, 'to': _to, 'value': value})
yield tx_hash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, doesn't seem to need to be a generator.

assert len(transaction_filter.get_new_entries()) == transaction_counter


@pytest.mark.xfail
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add the reason that this is a suspected eth-tester bug?

Copy link
Collaborator

@carver carver left a comment

Choose a reason for hiding this comment

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

LGTM!

@dylanjw dylanjw merged commit 51568c1 into ethereum:master Aug 16, 2018
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.

2 participants