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

evm_revert does not reset time adjustment #390

Closed
Recmo opened this Issue Sep 13, 2017 · 14 comments

Comments

Projects
None yet
10 participants
@Recmo

Recmo commented Sep 13, 2017

Expected Behavior

(Deploy contracts)
evm_snapshot
(Pass first half of test A)
evm_increaseTime
(Pass second half of test A)
evm_revert
(Pass test B)

Current Behavior

(Deploy contracts)
evm_snapshot
(Pass first half of test A)
evm_increaseTime
(Pass second half of test A)
evm_revert
(Fail test B)

Cause: evm_revert did not reset the time adjustment. Test B is now run in a different time than when the snapshot was taken, and fails.

Possible Solutions

  • Store time adjustment as part of snapshot.
  • Allow negative values in increateTime.
  • Allow setting time adjustment directly.
  • Allow setting time directly (added advantage of making tests more deterministic).

Context

I'm trying to make my unit tests run faster by re-using setup transactions using snapshots.

Your Environment

  • Version used: v4.1.1
  • Environment name and version (e.g. PHP 5.4 on nginx 1.9.1): node v8.4.0
  • Server type and version: github.com
  • Operating System and version: Linux 4.10.0-33-generic #37-Ubuntu SMP x86_64 GNU/Linux
  • Link to your project: https://github.com/Neufund/ico-contracts
@satyamakgec

This comment has been minimized.

Show comment
Hide comment
@satyamakgec

satyamakgec Sep 15, 2017

I am having same problem , How do i resolve it .
Environment
ubuntu 16.04.LTS
testrpc v4.0.1
truffle v3.4.5

satyamakgec commented Sep 15, 2017

I am having same problem , How do i resolve it .
Environment
ubuntu 16.04.LTS
testrpc v4.0.1
truffle v3.4.5

@scnale

This comment has been minimized.

Show comment
Hide comment
@scnale

scnale Sep 22, 2017

You may wish to take a look at the pull request I created here. The fix is quite simple but I'm not sure it's the best way to do it. You can try that out in the meantime though.

scnale commented Sep 22, 2017

You may wish to take a look at the pull request I created here. The fix is quite simple but I'm not sure it's the best way to do it. You can try that out in the meantime though.

@jaddison

This comment has been minimized.

Show comment
Hide comment
@jaddison

jaddison Oct 16, 2017

@scnale How does one go about building testrpc using your fix, exactly? Thanks!

jaddison commented Oct 16, 2017

@scnale How does one go about building testrpc using your fix, exactly? Thanks!

@scnale

This comment has been minimized.

Show comment
Hide comment
@scnale

scnale Oct 16, 2017

I haven't been using it in the last few weeks so the setup may need a few tweaks but here's what I did to test it:

  1. Get a copy of the working tree of the master branch of ganache-core with the fix commit included in it (at this time there may be new commits in master that are probably worth merging too). This can be easily achieved cloning both the pull request branch and master branch into a single local repository and merging master into the pull request branch.
  2. Get a copy of the working tree of the master branch of testrpc. Edit the devDependencies in the package.json(*) and replace the ganache-core version number with the path to the working tree of the ganache-core copy prepared in the previous step.
  3. To launch testrpc you can execute node <path to testrpc working tree>/cli.js [list of testrpc arguments that you want to use].

(*) I think there is a cleaner way to do this with npm link. It should be able to create a symbolic link in the node_modules subfolder to whichever path you choose. But I haven't really tested that out because I didn't know about npm link at the time. You could also try manually creating the symbolic link you need if you prefer.

Creating a symbolic link or alias in your terminal to launch the custom testrpc may be a good idea if you're planning on using it frequently.

Edit: Satisfying the other dependencies in both packages is necessary for this to work and prone to errors in my limited experience using npm. Use whatever means you deem adequate to satisfy them.

scnale commented Oct 16, 2017

I haven't been using it in the last few weeks so the setup may need a few tweaks but here's what I did to test it:

  1. Get a copy of the working tree of the master branch of ganache-core with the fix commit included in it (at this time there may be new commits in master that are probably worth merging too). This can be easily achieved cloning both the pull request branch and master branch into a single local repository and merging master into the pull request branch.
  2. Get a copy of the working tree of the master branch of testrpc. Edit the devDependencies in the package.json(*) and replace the ganache-core version number with the path to the working tree of the ganache-core copy prepared in the previous step.
  3. To launch testrpc you can execute node <path to testrpc working tree>/cli.js [list of testrpc arguments that you want to use].

(*) I think there is a cleaner way to do this with npm link. It should be able to create a symbolic link in the node_modules subfolder to whichever path you choose. But I haven't really tested that out because I didn't know about npm link at the time. You could also try manually creating the symbolic link you need if you prefer.

Creating a symbolic link or alias in your terminal to launch the custom testrpc may be a good idea if you're planning on using it frequently.

Edit: Satisfying the other dependencies in both packages is necessary for this to work and prone to errors in my limited experience using npm. Use whatever means you deem adequate to satisfy them.

@jaddison

This comment has been minimized.

Show comment
Hide comment
@jaddison

jaddison Oct 16, 2017

Thanks, @scnale - got it 'working'. some dependencies failed to install, but it still works and all test cases work now. Cheers!

jaddison commented Oct 16, 2017

Thanks, @scnale - got it 'working'. some dependencies failed to install, but it still works and all test cases work now. Cheers!

davidyuk added a commit to aeternity/aepp-response that referenced this issue Oct 17, 2017

Add Question contract
Increased time cannot be reseted according to trufflesuite/ganache-cli#390

davidyuk added a commit to aeternity/aepp-response that referenced this issue Oct 17, 2017

Add Question contract
Increased time cannot be reseted according to trufflesuite/ganache-cli#390

davidyuk added a commit to aeternity/aepp-response that referenced this issue Oct 17, 2017

Add Question contract
Increased time cannot be reseted according to trufflesuite/ganache-cli#390
@Zolmeister

This comment has been minimized.

Show comment
Hide comment
@Zolmeister

Zolmeister Oct 18, 2017

patch file

patch node_modules/ethereumjs-testrpc/build/cli.node.js testrpc-time.patch
83416c83416,83417
<       blockNumber: blockNumber
---
>       blockNumber: blockNumber,
> 			timeAdjustment: self.blockchain.timeAdjustment
83438a83440
> 	var timeAdjustment = this.snapshots[snapshot_id].timeAdjustment;
83465a83468,83469
> 		// The time adjustment is restored to its prior state
>  		self.blockchain.timeAdjustment = timeAdjustment;

Zolmeister commented Oct 18, 2017

patch file

patch node_modules/ethereumjs-testrpc/build/cli.node.js testrpc-time.patch
83416c83416,83417
<       blockNumber: blockNumber
---
>       blockNumber: blockNumber,
> 			timeAdjustment: self.blockchain.timeAdjustment
83438a83440
> 	var timeAdjustment = this.snapshots[snapshot_id].timeAdjustment;
83465a83468,83469
> 		// The time adjustment is restored to its prior state
>  		self.blockchain.timeAdjustment = timeAdjustment;

davidyuk added a commit to davidyuk/education-hackathon-2017 that referenced this issue Nov 8, 2017

Add Question contract
Increased time cannot be reseted according to trufflesuite/ganache-cli#390

davidyuk added a commit to aeternity/aepp-response-contracts that referenced this issue Nov 30, 2017

Add Question contract
Increased time cannot be reseted according to trufflesuite/ganache-cli#390

davidyuk added a commit to aeternity/aepp-response-contracts that referenced this issue Nov 30, 2017

Add Question contract
Increased time cannot be reseted according to trufflesuite/ganache-cli#390
@cedricwalter

This comment has been minimized.

Show comment
Hide comment
@cedricwalter

cedricwalter Jan 16, 2018

status? will it be in soon?

cedricwalter commented Jan 16, 2018

status? will it be in soon?

@benjamincburns

This comment has been minimized.

Show comment
Hide comment
@benjamincburns

benjamincburns Jan 16, 2018

Collaborator

There's an open PR on this trufflesuite/ganache-core#2. Last I knew the changes @scnale submitted there had problems that needed fixing (see this comment). I'll double check, but I don't think that's been done yet. If someone wants to submit another PR that finishes the work, I'd be all for it.

Collaborator

benjamincburns commented Jan 16, 2018

There's an open PR on this trufflesuite/ganache-core#2. Last I knew the changes @scnale submitted there had problems that needed fixing (see this comment). I'll double check, but I don't think that's been done yet. If someone wants to submit another PR that finishes the work, I'd be all for it.

@benjamincburns

This comment has been minimized.

Show comment
Hide comment
@benjamincburns

benjamincburns Feb 27, 2018

Collaborator

@scnale seems to have ghosted, and the changes he submitted were incomplete. If anyone else wants to take this on, I'd be much obliged.

Collaborator

benjamincburns commented Feb 27, 2018

@scnale seems to have ghosted, and the changes he submitted were incomplete. If anyone else wants to take this on, I'd be much obliged.

@roderik

This comment has been minimized.

Show comment
Hide comment
@roderik

roderik Feb 27, 2018

Funny, just looking for a solution to this issue as well :)
Anyone knows a workaround?

roderik commented Feb 27, 2018

Funny, just looking for a solution to this issue as well :)
Anyone knows a workaround?

@PhABC

This comment has been minimized.

Show comment
Hide comment
@PhABC

PhABC Apr 2, 2018

Just made a very simple PR that helped me solve the problem for certains tests ; trufflesuite/ganache-core#95

PhABC commented Apr 2, 2018

Just made a very simple PR that helped me solve the problem for certains tests ; trufflesuite/ganache-core#95

@PhABC

This comment has been minimized.

Show comment
Hide comment
@PhABC

PhABC Apr 2, 2018

In addition, the PR mentioned above works as expected (see comment #377928638).

PhABC commented Apr 2, 2018

In addition, the PR mentioned above works as expected (see comment #377928638).

@benjamincburns

This comment has been minimized.

Show comment
Hide comment
@benjamincburns

benjamincburns Apr 12, 2018

Collaborator

Fixed in current develop - thanks for following up on this, @PhABC!

Collaborator

benjamincburns commented Apr 12, 2018

Fixed in current develop - thanks for following up on this, @PhABC!

@adamsoffer

This comment has been minimized.

Show comment
Hide comment
@adamsoffer

adamsoffer Apr 29, 2018

I'm also running into this issue. Which version of ganach-cli has the fix?

adamsoffer commented Apr 29, 2018

I'm also running into this issue. Which version of ganach-cli has the fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment