Remove moment.js dependencies #368
Conversation
There was a problem hiding this comment.
Great work!
There are a lot of changes in yarn.lock which are unrelated to removing moment.js. Can you look into that? Make sure to have your dependencies as specified after #363. Sorry for the confusion. As for keeping package-lock.json in sync, it suffices to remove the "moment" entry.
test/helpers/increaseTime.js
Outdated
| @@ -43,5 +43,7 @@ export const duration = { | |||
| minutes: function(val) { return val * this.seconds(60) }, | |||
| hours: function(val) { return val * this.minutes(60) }, | |||
| days: function(val) { return val * this.hours(24) }, | |||
| weeks: function(val) { return val * this.days(7) } | |||
| weeks: function(val) { return val * this.days(7) }, | |||
| months: function(val) { return val * this.days(30)}, | |||
There was a problem hiding this comment.
I'm thinking we should only provide time units matching those of Solidity. months is not included and I see you haven't used it anywhere either. What do you think about removing it?
There was a problem hiding this comment.
Yes, it makes a total sense to mimic the solidity constants. Thanks for pointing that out!
| @@ -29,26 +28,26 @@ contract('TokenTimelock', function ([_, owner, beneficiary]) { | |||
| }) | |||
|
|
|||
| it('cannot be released just before time limit', async function () { | |||
| await increaseTime(moment.duration(0.99, 'year').asSeconds()) | |||
| await increaseTimeTo(this.releaseTime - duration.seconds(3)) | |||
There was a problem hiding this comment.
I'm surprised this works. Given the lack of precision of increaseTimeTo that we've discussed, I would've thought 3 seconds was not enough to make up for it. Did you try other values?
There was a problem hiding this comment.
As testrpc is run on localhost the lag is usually up to 1s. Due to time rounding, subtracting a single second is not enough to have a robust behaviour but 3s seems to be safe enough.
Obviously, the situation may change if someone decides to run tests across testrpc instance connected. But, to be honest, I don't expect it to be a common practice.
|
@frangio I've included all of the latest changes and generated a new yarn.lock. I suppose the inconsistency is caused by us using different versions of yarn. Can you please tell me yours (mine is 0.24.5) so I can double check if that's the case before committing updates. |
|
yarn version 0.28.4 You should remove your |
|
Switching to version 0.28.4 helped. |
|
Excellent. Thanks @jakub-wojciechowski! |
Remove moment.js dependencies
Following the discussion with @frangio in PR #353
As
testrpcuses plain time arithmetic for time manipulation, using moment.js which is based on the implementation of full calendar logic brings a potential risk of indeterministic errors in tests.