Add output to file feature #47
Conversation
Codecov Report
@@ Coverage Diff @@
## main #47 +/- ##
==========================================
+ Coverage 82.29% 91.64% +9.34%
==========================================
Files 19 20 +1
Lines 305 323 +18
==========================================
+ Hits 251 296 +45
+ Misses 54 27 -27
Continue to review full report at Codecov.
|
| parser = Skunk::Cli::Options::Argv.new(argv) | ||
| parser.parse | ||
| _(parser.output_filename).must_equal "file.txt" | ||
| end |
There was a problem hiding this comment.
@manuca Could you add another check to make sure that the content of file.txt is the right content?
There was a problem hiding this comment.
@etagwerker When I created this test I thought about this but realized the report output seems difficult to make deterministic because of the dependency on git commits for instance. I saw we created and injected a mock status reporter to make the output deterministic in status_reporter_test.rb but I don't see this possible at this level of testing.
Do you have ideas on how it would be best to test the output at this level?
There was a problem hiding this comment.
@manuca you can probably get around the non-deterministic problem with git commits by stubbing the method that calculates the churn of the file.
| FileUtils.rm("tmp/generated_report.txt", force: true) | ||
| FileUtils.mkdir_p("tmp") | ||
|
|
||
| RubyCritic::AnalysedModule.stub_any_instance(:churn, 1) do |
There was a problem hiding this comment.
@etagwerker I added these stubs to address your comments about testing the generated content in the files. Note I applied these at the level of the application that is where the file is output, not in the argv parser as you suggested.
test/lib/skunk/application_test.rb
Outdated
| end | ||
| end | ||
|
|
||
| _(File.read("tmp/generated_report.txt")).must_equal File.read("test/samples/console_output.txt") |
There was a problem hiding this comment.
@etagwerker @bronzdoc I've been banging my head with this assertion, I can't make it pass.
I'm not being able to read any contents from tmp/generated_report.txt, they are consistently empty ("").
After the tests finish I can confirm the contents are flushed to the file, but not within the test. This is weird. I tried doing #flush on the output stream without success. I also tried using the fsync method of IO, but nothing.
Do you have any ideas? 🦆
This was the flush call I added, but now removed:
9e11eec#diff-fc8fbe9f52913e11298c9a0d6a140818L23
There was a problem hiding this comment.
@bronzdoc what happened with those commits, I noticed the history got rewritten on this branch. Did you do a rebase against master? Next time please let's get on the same page before doing commits that break history on branches we might be sharing to avoid conflicts.
Regarding the assertion that wasn't passing on my test, did you figure an explanation for why it wasn't working? I noticed test are green now.
There was a problem hiding this comment.
@manuca what commits? the commits are there. I just removed one commit that I made that was not needed. Beside that I fixed the merge conflicts and a force push had to be made.
I think the issue was that we were returning a stream/file in the method but we never closed it. So i guess while is opened we cannot read it.
def output_stream
output_filename = @argv_options.output_filename
output_filename.nil? ? $stdout : File.open(output_filename, "w")
endeverytime we used output_stream it will return a new file object so even if we do something like this:
output_stream.puts "hello world"
output_stream.closethat will not close the file created the first time when we do output_stream.puts "hello world"
There was a problem hiding this comment.
I tried closing the output stream explicitly when we were handling output to a file as well as flushing the stream but couldn't make it work either way so I didn't think it had to do with that, very strange issue.
bronzdoc
force-pushed
the
output-to-file
branch
from
c532ef3 to
58125a3
Compare
This reverts commit c72ca61.
bronzdoc
force-pushed
the
output-to-file
branch
from
359fb31 to
1f1f594
Compare
| if filename.nil? | ||
| $stdout.puts(message) | ||
| else | ||
| File.open(filename, "w") { |file| file.puts(message) } |
There was a problem hiding this comment.
@bronzdoc My only concern is that with this approach we're opening and closing immediately the file for each line we output.
There was a problem hiding this comment.
We are not writing line by line we just open an IO object and write a complete string to it. But maybe I'm missing something?
There was a problem hiding this comment.
🤔 I think you're right.
There was a problem hiding this comment.
@manuca Looks good! Could you add an entry to this file? https://github.com/fastruby/skunk/blob/main/CHANGELOG.md
Thanks!
|
@etagwerker added the entry to the CHANGELOG, I think it is ready for merging then. |
|
@manuca Great, thanks! |
Adds output to file option, usage:
Fixes #45