Minimize changes to test/contract.js (#3190) #3303
Conversation
cgewecke
changed the title
| var eth = new Eth(provider); | ||
| const contract = new eth.Contract(abi, address, options); | ||
| //eth.setProvider(provider); |
There was a problem hiding this comment.
The call of setProvider is not required because the constructor of the Eth module does set the provider already as expected. The tests fail because the added validations to the FakeIpcProvider aren't in the correct order (I will check that closer). But we can say that the logic behind setProvider is still correct or the test case on line 3112 would fail.
There was a problem hiding this comment.
Yes, it seems possible everything is ok but it would be good to certain as well :)
There was a problem hiding this comment.
@cgewecke I've checked the issue we have with the additional call of eth.setProvider closer. Because we pass the exact same provider again with setProvider does it register the data listener again and the eth_getTransactionReceipt call will get executed more than required. We could implement the removal of all listeners on setProvider to prevent this behaviour but this would probably break existing DApps which do register event listeners before setProvider got called.
After all, if we add an explanatory note to the setProvider method to inform the user about this behaviour can we ignore it. WDYT?
There was a problem hiding this comment.
Ok - that sounds reasonable...I'm going to walk the code again as you describe just to satisfy my paranoia. The way the test set's itself up with with two provider settings doesn't seem very normal anyway.
… if someone does passes the exact same provider instance again to the setProvider method of the current module
|
@cgewecke I've extended the note of the |
Description
PR targets #3190
(Part of ongoing #3190 review. Opening for further discussion.)
PR reverts all the changes to test/contract.js except the three necessary to get the tests passing.
Diff vs. 1.x looks like this:
Many tests fail unless the call to eth.setProvider is commented out in the test fixture. Why?
https://github.com/ethereum/web3.js/blob/5e1aec2f07c82d412123229bef8a865142a8668a/test/contract.js#L242-L244