-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 tempfile and datapath helpers to specs #5951
Add tempfile and datapath helpers to specs #5951
Conversation
f16066c
to
bf2f07c
Compare
Looks good! Could you add a little doc for |
@@ -97,138 +97,102 @@ describe "FileUtils" do | |||
|
|||
describe "touch" do | |||
it "creates file if it doesn't exists" do | |||
filename = File.join(__DIR__, "data/test_touch.txt") | |||
begin | |||
File.exists?(filename).should be_false |
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.
you removed this check, is it intentional?
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.
with_tempfiles
should guarantee that the path does not exist, don't need to check that again.
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.
Let's keep it just to make sure.
spec/std/file_utils_spec.cr
Outdated
] | ||
begin | ||
it "creates multiple files if they don't exists" do | ||
with_tempfile("file_utils-touch") do |path_prefix| |
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.
might be a problem here: you'll have 3 files created that don't get cleaned after with_tempfile
, maybe use sth like:
with_tempfile("touch_1.txt", "touch_2.txt", "touch_3.txt") do |*paths|
# ...
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.
true, that needs to be refactored.
spec/std/file_utils_spec.cr
Outdated
end | ||
|
||
it "tests rm with non existing path" do | ||
expect_raises Errno do | ||
FileUtils.rm("/tmp/crystal_rm_test_#{Process.pid}") | ||
with_tempfile("rm-nonexstinent") do |path| |
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.
typo: non-existent
handler.call context | ||
response.close | ||
io.rewind | ||
HTTP::Client::Response.from_io(io, ignore_body) | ||
end | ||
|
||
describe HTTP::StaticFileHandler do | ||
file_text = File.read "#{__DIR__}/static/test.txt" | ||
file_text = File.read datapath("static_file_handler", "test.txt") |
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.
maybe memorize datapath("static_file_handler", "test.txt")
in a static_file_path
variable for all the specs below that uses 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.
I'm not sure that's worth the indirection.
b721d35
to
94b782a
Compare
Damn, the temp paths are too long for |
94b782a
to
62951af
Compare
@bew I added a comment to
|
On OSX it seems that Any ideas on this? If we just set |
321a038
to
0812ca2
Compare
Apart from the OSX issue this is mostly complete. The remaining instances of
|
026871f
to
26c7c16
Compare
26c7c16
to
f15d235
Compare
It would be beneficial if this PR could get reviewed and hopefully merged soon. It touches a lot of code in the spec suite, but it's mostly insignificant changes. I'd like to avoid lots of upcoming rebasing in this and future PRs. Any idea on the OSX issue @RX14? |
If |
@RX14 Do you mean generally, in IMHO the cleanest solution would be to set |
Fixing things in the CI script is always wrong because you're admitting you're going to break the specs on everyone's osx laptops. I meant change |
But why? If a user consciously sets We could think about overwriting the temp dir used by You could see it that way: The specs for Unix sockets puts them in a sub path of Let's consider the downsides: If we change the behaviour of If we set However, this absolutely needs to be documented. Unix sockets will also fail in applications using a Changing the spec suite to adapt to a special case is not a good solution for a generic problem. We should rather improve the error message for Unix sockets failing because of path length. Instead of |
Specs failing is very much a "real problem"... |
It's not documented but I'd say it's just missing documentation. The specs for And again, I see no sane reason to generally ignore the environment variable if we think it's to long for one specific use case. |
Let's just fix it by hard coding to /tmp on osx in with_tempfile |
I really don't like that To get this moving forward, what do others think? /cc @crystal-lang |
well, the fact is that |
|
I use Note i'm assuming that people using osx have a really long |
It would only break if the Unix socket specs are run and There are countless ways to do that. A few suggestions: change global config, apply context-sensitive in crystal project folders, explicitly And I'd improve the error message for Unix sockets failing because of path length, including an advice to change |
@RX14 I'll try it with a shorter tempfile subpath. Let's see if it works. |
if @path.bytesize + 1 > MAX_PATH_SIZE we're one off. |
da87116
to
b874560
Compare
b874560
to
5ae1347
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.
I found some issues and some doubts, but is almost right.
spec/spec_helper.cr
Outdated
@@ -153,7 +154,7 @@ class Crystal::SpecRunOutput | |||
end | |||
end | |||
|
|||
def run(code, filename = nil, inject_primitives = true, debug = Crystal::Debug::None) | |||
def run(code, filename = nil, inject_primitives = true, debug = Crystal::Debug::None, *, file = __FILE__) |
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 specs doesn't care about the location of this files.
They are not even generated sometimes (JIT is used unless prelude is required).
I would avoid adding this named arg and just using a fixed tempfile prefix/sufix in this helper.
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'm not sure I understand why you'd want to remove this. Maybe it's not strictly necessary, but using with_tempfile
here certainly doesn't hurt and it's easier than managing a tempfile location manually.
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.
As I said, if specs are failing I would probably run just a few or one. Or even add a debug message for the spec/tempfile association while running specs in some verbose mode.
Having filename
and file
arguments in the same method seems misleading.
So, between not been able see an addition here, plus the misleading name, plus having more data to cary on runtime on the high populated spec suite of the compiler, I would vote for removing 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.
Okay, I'll revert it.
spec/spec_helper.cr
Outdated
# write code to the temp file | ||
File.write(code_file.path, code) | ||
def build_and_run(code, *, file = __FILE__) | ||
with_tempfile("build_and_run_code", "build_and_run_bin", file: file) do |code_path, binary_path| |
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.
ditto
spec/spec_helper.cr
Outdated
o_filename = "#{__DIR__}/temp_abi.o" | ||
begin | ||
File.write(c_filename, c_code) | ||
def test_c(c_code, crystal_code, *, file = __FILE__) |
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.
ditto
File.join(".", "spec", "std", "data", "dir", "dots", ""), | ||
File.join(".", "spec", "std", "data", "dir", "subdir", ""), | ||
File.join(".", "spec", "std", "data", "dir", "subdir2", ""), | ||
].sort | ||
end | ||
|
||
it "tests with relative path (starts with ..)" do | ||
base_path = File.join("..", File.basename(File.dirname(File.dirname(__DIR__))), "spec", "std", "data", "dir") | ||
Dir["../#{File.basename(File.dirname(File.dirname(__DIR__)))}/spec/std/data/dir/*/"].sort.should eq [ | ||
base_path = File.join("..", File.basename(Dir.current), "spec", "std", "data", "dir") |
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 would think that relaying on __DIR__
is better that Dir.current
since the later involves more state.
Why this change?
I also notice that the datapath
helper relies on the current directory and not an absolute path by using __DIR__
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.
__DIR__
is compile time, Dir.current
runtime. In order for the specs to correctly execute when cross-compiled, we need to use Dir.current
because __DIR__
might not even exist on the target system.
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, understood.
spec/std/file_spec.cr
Outdated
end | ||
|
||
it "gives false" do | ||
File.file?("#{__DIR__}/data").should be_false | ||
File.file?(datapath("nonexistent_dir")).should be_false |
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 should point to an existing dir
spec/std/file_spec.cr
Outdated
@@ -594,7 +578,7 @@ describe "File" do | |||
it "converts a pathname to an absolute pathname, using ~ (home) as base (trailing /)" do | |||
prev_home = home | |||
begin | |||
ENV["HOME"] = __DIR__ + "/" | |||
ENV["HOME"] = File.expand_path(datapath("", "")) |
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.
Why the double "", ""
?
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.
Not sure... probably left over.
Merge? (before this PR needs yet another rebase...) |
I think that after this PR the
|
True. The |
I disagree. It's faster to compile combined. |
I think we should have 2 datapath helpers also. |
This PR introduces
with_tempfile
anddatapath
helpers to the stdlib spec suite.with_tempfile
receives a path and expands it relative to a temporary directory unique to every spec run. All temporary paths are prefixed by the name of the spec file that requests them. The constructed path is yielded and cleaned up after the block has returned. It can be used as file or directory path. The unique directory is removed at the end of the spec run (unless env varSPEC_TEMPFILE_CLEANUP
is set to0
- which can be useful for debugging).datapath
is used to create platform-independent paths tospec/std/data
(orspec/compiler/data
in the compiler spec).Closes #5811
This is work in progress. Feedback is still welcome.