-
Notifications
You must be signed in to change notification settings - Fork 7
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
[ST-1312] Added dynamic loading support #205
base: master
Are you sure you want to change the base?
Conversation
test/lib/services/time_test.rb
Outdated
current_time = Time.now.utc.sec | ||
assert((TimeUtil.time_proc.call - current_time) < 2.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test doesn't look useful. There's not any logic here, it is just a wrapper for Time.now
so i don't think you need to test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this test might seem very redundant. But this is the only check that makes sure the time_proc
is usable (we mocked it in all other tests). It's basically to make sure 1) the proc is actually callable and it is a proc and 2) it's not returning something totally unexpected (like a datetime object for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timecop would still be useful if you wanted to write this more strictly 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed time_proc and tested with Timecop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Purpose/goal of Pull Request (Issue #)
https://wovnio.atlassian.net/browse/ST-1312
Comments