-
-
Notifications
You must be signed in to change notification settings - Fork 388
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
Add microseconds specs for File.mtime, .atime and .ctime #386
Conversation
351f3ed
to
1321c46
Compare
What can I do with these fails? On my computer:
|
The failures are from the Windows CI, because I guess Windows does not have micro-second resolution for file times. You can wrap the new specs in |
core/file/atime_spec.rb
Outdated
@atime = File.atime(@file) | ||
break if @atime.usec > 0 | ||
sleep 0.1 | ||
end |
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.
Please move this directly in the concerned spec as other specs do not need this, and the sleep wastes time.
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.
before :each
is executed before each it
block.
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.
Done.
core/file/ctime_spec.rb
Outdated
@ctime = File.ctime(@file) | ||
break if @ctime.usec > 0 | ||
sleep 0.1 | ||
end |
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.
ctime will not change just by retrying, creating another file with touch(tmp("ctime"))
would be more helpful.
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.
Done.
core/file/atime_spec.rb
Outdated
3.times do | ||
@atime = File.atime(@file) | ||
break if @atime.usec > 0 | ||
sleep 0.1 |
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.
Sleep only a millisecond, it's more than enough to change microseconds.
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.
Done.
@eregon Why I think that condition by OS is incorrect: this can be not OS-specific, but FS-specific — rubinius/rubinius#3502 (comment) We don't have builds on OS X, do we? So… it's embarrassing situation. |
@AlexWayfer Right, so both platforms should be excluded then, but please verify if you can. We had OS X builds on Travis, but they are super slow so I pragmatically removed them. In any case it is becoming obvious that this behavior is platform-dependent. |
I checked: OS X failed :(
Yes, you right. I found And I'm worried about Linux running on file systems where there is no support for microseconds. But OK, I will add "whitelist platforms" check. |
Reference: jruby/jruby#4520 (comment) Three attemps to avoid zero in these values with small sleep (if failed). And some code refactoring.
1321c46
to
9aba5c7
Compare
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. Consider increasing number of loops to reduce likelihood we hit zero usec three times in a row (which is pretty unlikely, but this is also pretty cheap to do a few more times.)
@@ -15,6 +15,17 @@ | |||
File.atime(@file).should be_kind_of(Time) | |||
end | |||
|
|||
platform_is :linux do | |||
it "returns the last access time for the named file with microseconds" do | |||
3.times do |
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.
The cost of doing this repeatedly is pretty low, so this might as well be 10 or 100. But I support the idea in general; this was my stumbling block as well when I started to think about a spec.
The alternative would be to create a temporary file and forcibly modify these times, but I don't know if that's even possible for atime.
Note also that some Linux systems disable atime (e.g. via mount params) for better filesystem speed. I don't know if that will ever affect specs, though.
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.
There is already only 1/1000 chance per iteration to get 0 (assuming millisecond granularity), so I think it's fine.
The second iteration will be even more unlikely to get 0 as the work in between will not take exactly one second.
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.
The cost of doing this repeatedly is pretty low, so this might as well be 10 or 100.
Increased to 100.
The alternative would be to create a temporary file and forcibly modify these times, but I don't know if that's even possible for atime.
That's possible for atime
and mtime
via FileUtils#touch.
This alternative was used.
Note also that some Linux systems disable atime (e.g. via mount params) for better filesystem speed. I don't know if that will ever affect specs, though.
This comment added to code, if you don't mind.
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.
Oh, it's already merged… What can I do?
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 think it's OK as it is (that's why I merged it), but if you want to change please feel free to open another Pull Request.
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.
OK, thank you!
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 look great, thank you for the new specs!
@@ -14,6 +14,21 @@ | |||
File.ctime(@file).should be_kind_of(Time) | |||
end | |||
|
|||
platform_is :linux do | |||
it "Returns the change time for the named file (the time at which directory information about the file was changed, not the file itself) with microseconds." do |
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.
Small typo, specs descriptions should start with a lowercase letter.
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.
Copy-pasted from previous it
:(
What can I do with some fixes if that PR was merged? New PR?
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 fixed it along with a few other cases in c5de600
@@ -15,6 +15,17 @@ | |||
File.atime(@file).should be_kind_of(Time) | |||
end | |||
|
|||
platform_is :linux do | |||
it "returns the last access time for the named file with microseconds" do | |||
3.times do |
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.
There is already only 1/1000 chance per iteration to get 0 (assuming millisecond granularity), so I think it's fine.
The second iteration will be even more unlikely to get 0 as the work in between will not take exactly one second.
Reference: jruby/jruby#4520 (comment)
Three attemps to avoid zero in these values with small sleep (if failed).
And some code refactoring.