Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

Nc 1145 Acceptance test for getTransactionReceipt JSON-RPC method #278

Merged
merged 20 commits into from
Nov 20, 2018

Conversation

Shinabyss
Copy link
Contributor

PR description

Write acceptance test for JSON-RPC method getTransaction Receipt.
Test only attempt to test the method to retrieve the transaction receipt given the transaction hash.
The correctness of the receipt (e.g sender's address, recipient address, gas cost) is not tested here.

Fixed Issue(s)

https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_gettransactionreceipt

@CLAassistant
Copy link

CLAassistant commented Nov 19, 2018

CLA assistant check
All committers have signed the CLA.

public void mustCorrectlyQueryTransactionReceiptFromContractCreationTransaction() {
final Hash transactionHash = minerNode.execute(transactions.createTransfer(recipient, 5));
cluster.verify(recipient.balanceEquals(5));
minerNode.verify(eth.getTransactionReceipt(transactionHash.toString()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming-wise this would probably be better as:
minerNode.verify(eth.transactionReceiptIsAvailable(transactionHash))

You shouldn't need to call toString() in the test - the DSL should accept the actual Hash and convert to a string itself if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed and moved toString() into DSL

public void mustReturnNullWhenTransactionDoesNotExist() {
minerNode.verify(
eth.getTransactionReceiptNull(
"0xb903239f8543d04b5dc1ba6579132b143087c68db1b2168786408fcbce568238"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably would be better as minerNode.verify(eth.transactionReceiptNotAvailable("...")). Using a string here is probably sensible since the test is simpler that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed test to be more succinct


import org.web3j.protocol.core.methods.response.TransactionReceipt;

public class SanityCheckEthGetTransactionReceiptValues implements Condition {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't doing any sanity checking - just checking it's present so probably should just be ExpectEthGetTransactionReceiptPresent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

return new SanityCheckEthGetTransactionReceiptValues(transactions.getTransactionReceipt(param));
}

public Condition getTransactionReceiptNull(final String param) {
Copy link
Contributor

Choose a reason for hiding this comment

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

param should have a meaningful name - maybe transactionHash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

Copy link
Contributor

@CjHare CjHare left a comment

Choose a reason for hiding this comment

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

LGTM

@Shinabyss Shinabyss merged commit c555993 into PegaSysEng:master Nov 20, 2018
@Shinabyss Shinabyss deleted the NC-1145 branch November 26, 2018 17:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants