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.

Sign up for GitHub

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

Jump to bottom

thegraph-ethereum: Mock transport for testing, test contract_call #127

Merged
merged 3 commits into from
Jun 29, 2018
Merged

thegraph-ethereum: Mock transport for testing, test contract_call #127

merged 3 commits into from
Jun 29, 2018

Conversation

adklempner
Copy link
Contributor

No description provided.

@ghost ghost assigned adklempner Jun 28, 2018
Zerim
Zerim reviewed Jun 29, 2018
View reviewed changes
thegraph-ethereum/src/lib.rs Outdated
}],
constant: true,
};
let function = Function::from(balance_of_function);
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary. balance_of_function is already a Function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in latest commit

@Zerim
Copy link
Contributor

Zerim commented Jun 29, 2018

Let's move the TestTransport out of src and into a tests folder, keeping consistent with our other crates. Either there, or in thegraph-mock if we think it could be more broadly useful in unit testing other crates or in writing integration tests.

Zerim
Zerim suggested changes Jun 29, 2018
View reviewed changes
Copy link
Contributor

@Zerim Zerim left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is a good start! We can assert on more interesting behavior over time, but I think this is good coverage for now, while we're moving fast. After the TestTransport implementation is moved, this will have a thumbs up from me.

@leoyvens
Copy link
Collaborator

Having all mocks in thegraph-mock might not be the best organization, we could have mocks be exported from their respective crates. We have everything abstracted with traits so it should not be hard to share the initialization logic between production and integration tests.

If we move the mock to tests then we can't export it to other crates, so I'd keep the organization as it is.

@adklempner
Copy link
Contributor Author

I opened an issue in rust-web3 re: publicly exporting web3::helpers::TestTransport. I think that would be the cleanest way to do this.

Zerim
Zerim approved these changes Jun 29, 2018
View reviewed changes
Copy link
Contributor

@Zerim Zerim left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@adklempner adklempner merged commit f166b92 into master Jun 29, 2018
@Jannis Jannis deleted the adklempner/contract-call-test branch July 26, 2018 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zerim Zerim Zerim approved these changes

Assignees

@adklempner adklempner

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adklempner @Zerim @leoyvens