Async/await tests throwing revert error randomly
#920
Comments
|
@kermankohli Hi, just peeked at one of your test files and found this. It's a You might comb through the code and make sure everything async/await is as it should be. |
|
Thanks for pointing that out! I removed it (checked for other references) and reran the tests although the same issue still persists: |
|
All those 3 tests were run straight after each other. Only the computed subscription hash test consistently fails. |
|
@kermankohli I wonder if it's related to the mix of If you change |
|
So I just tried getting rid of single |
|
@kermankohli Mmmm. Maybe something about the way the times are being computed? What if you slow those tests way down by inserting at the top of each? await setTimeout(() => new Promise((accept) => accept()), 1000) // Or something like this . . . |
|
Okay so gave that a go and the same 12 tests fail which is good since at leas there's consistency in what's not working. Although still not sure what exactly is going on here.
|
|
Lol ok great! |
|
Kind of - although why do you think the tests are failing randomly unless I have the delay? Can't see what's causing the race condition in this case. |
|
@kermankohli I don't know - it must be something about the time values as they execute. Maybe the resolution is too low? If you put logging statements in to see what they are, the bug would probably go away too because those introduce their own delay. |
|
The time values for when the subscription is being created or you mean something else? Apart from logging is there any cleaner way of doing this? Although throughout my program, all the time logic is to check a date in the past so I'm not sure how it's caused a race condition to occur. |
|
@kermankohli Does the contract rely on the block timestamp or |
|
Yup. In VolumeSubscription there's statements such as: |
|
@kermankohli Yes - this is a big issue. Effective resolution for block.timestamp is the amount of time it takes to mine a block. On mainnet 'many' seconds, on the test client, milliseconds. Ultimately the contract's idea of what time it is determines whether or not that require statement is true or not. |
|
So essentially the time taken to mine the block may be greater than the existing time captured in Javascript? |
|
There's some information about on how Solidity thinks block.timestamp should be understood. It also looks like the expected precision is 1 second and believe that ganache respects that so suggestion above about The safest thing might be to fetch the current block, read the timestamp, and generate a timestamp that is definitely earlier (or later) than what ganache thinks the time is. |
|
@kermankohli This is a very interesting topic today - more in depth discussion at #921 as well. |
|
@kermankohli Should we keep this open? |
|
@cgewecke Not sure to be honest. Theoretically the only test that should be failing are the ones that are ones that rely on time logic. However there's tests which simply read data and fail due to |
cgewecke
changed the title
revert error randomly
|
@kermankohli Ok, I'm going to rename this issue a little to narrow its scope. Please ping or close if you discover something. |
|
@kermankohli Did you ever get to the bottom of this? |
|
@cgewecke yup finally did 3 days ago! so essentially when require statements are dependent on time they can cause failures at places which seem random. the way i solved this was create a mock contract that inherited from the actual contract and overrides a |
|
Ok, well thanks for opening - I'm sure this will be helpful to others. Closing since you found a way out. |
|
@kermankohli - side note: I believe that you have a logical error in file https://github.com/8x-protocol/8x-protocol/blob/master/test/helpers/assert_revert.js. If You then catch this error, and since its message contains Instead, I suggest that you |
|
@barakman thanks for that although not sure what you mean. the function is meant to throw an exception if the promise executes since we didn't want it to execute in the first place. feel free to correct me if i'm wrong/misunderstanding you though :) |
|
@kermankohli: So, let's analyze a scenario in which the
In order to understand the logical confusion, keep in mind that there are two different failure scenarios which you need to detect and handle:
In your code, I believe that you detect the first scenario correctly, but you handle it incorrectly. |
|
@barakman ah - i get what you're saying although the "assert.throw" shows in the console and fails the tests so it doesn't matter if the catch block marks it as a success since it's already failed. i've manually tested out the code by providing it passing and failing conditions and it works as expected. also, the code is actually from OpenZepelin so might be worth discussing it with them however you should create a demo project and test it out for yourself first https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/test/helpers/assertRevert.js. |
|
OK, perhaps I am missing something. The thing to understand is that the Mocha framework reports a failure only when your test ( That said, here is my analysis of your code and why I think it handles a specific scenario incorrectly: Now, let's analyze the
In any case, I have posted it on Zeppelin GIT as well (here). |
|
@barakman Nice catch!! |
|
@cgewecke: Thank you, but keep in mind that the |
Issue
The following error causes tests to fail randomly.
Error: VM Exception while processing transaction: revert
Steps to Reproduce
It's hard to reproduce although here's a link to the repo:
https://github.com/kermankohli/ethme
Expected Behavior
Tests should consistently pass all the time.
Actual Results
The same test will fail, and then pass when rerun again. It seems to be some kind of caching issue or race condition.
Environment
truffle version): 4.1.7node --version): 9.10.1npm --version): 5.6.0The text was updated successfully, but these errors were encountered: