Permalink
Browse files

Recorder: Introduce Recorder and its subclasses

For now this is purely a matter of internal code organization. Next
commit will change this.
  • Loading branch information...
1 parent 958b25b commit 54357bf836fdd7d0d42644062d5acefb010394c8 @dmajda dmajda committed Jun 20, 2012
Showing with 154 additions and 45 deletions.
  1. +2 −0 cheetah.gemspec
  2. +59 −45 lib/cheetah.rb
  3. +93 −0 spec/cheetah_spec.rb
View
@@ -24,6 +24,8 @@ Gem::Specification.new do |s|
"lib/cheetah.rb"
]
+ s.add_dependency "abstract_method", "~> 1.2"
+
s.add_development_dependency "rspec"
s.add_development_dependency "redcarpet"
s.add_development_dependency "yard"
View
@@ -1,3 +1,4 @@
+require "abstract_method"
require "logger"
require "shellwords"
require "stringio"
@@ -69,17 +70,61 @@ def initialize(commands, status, stdout, stderr, message = nil)
end
# @private
- class LogAdapter
+ class Recorder
@jreidinger

jreidinger Jun 21, 2012

Member

I think this class is not needed at all. We can use as default class or documentation class Null Recorder and user can define what he need

@dmajda

dmajda Jun 21, 2012

Contributor

I see NullRecorder as a special kind of recorder, not a place where recorder interface should be described.

But I'll think about this. Maybe it would be enough to describe the Recorder interface somewhere in Cheetah.run docs and we can get rid of the class.

+ abstract_method :initialize
@jreidinger

jreidinger Jun 21, 2012

Member

Do not force user to have initialize. It is up to him how he want to set his instance

@dmajda

dmajda Jun 21, 2012

Contributor

I guess you're right, fixed.

+ abstract_method :record_commands
+ abstract_method :record_stdin
+ abstract_method :record_status
+ abstract_method :record_stdout
+ abstract_method :record_stderr
+ end
+
+ # @private
+ class NullRecorder < Recorder
+ def initialize(logger); end
@jreidinger

jreidinger Jun 21, 2012

Member

useless here. No initializer needed

@dmajda

dmajda Jun 21, 2012

Contributor

Again, fixed.

+ def record_commands(commands); end
+ def record_stdin(stdin); end
+ def record_status(status); end
+ def record_stdout(stdout); end
+ def record_stderr(stderr); end
+ end
+
+ # @private
+ class DefaultRecorder < Recorder
def initialize(logger)
@jreidinger

jreidinger Jun 21, 2012

Member

that first useful initializer, you need logger, then set it in initialization

@logger = logger
end
- def info(message)
- @logger.info(message) if @logger
+ def record_commands(commands)
+ @logger.info "Executing #{format_commands(commands)}."
+ end
+
+ def record_stdin(stdin)
+ @logger.info "Standard input: #{format_input_output(stdin)}"
+ end
+
+ def record_status(status)
+ @logger.send status.success? ? :info : :error,
+ "Status: #{status.exitstatus}"
end
- def error(message)
- @logger.error(message) if @logger
+ def record_stdout(stdout)
+ @logger.info "Standard output: #{format_input_output(stdout)}"
+ end
+
+ def record_stderr(stderr)
+ @logger.send stderr.empty? ? :info : :error,
+ "Error output: #{format_input_output(stderr)}"
+ end
+
+ private
+
@jreidinger

jreidinger Jun 21, 2012

Member

I think that these methods shouldn't be private. At least make it protected as this is good base class if you want to hide some information in log or have different log message.

@dmajda

dmajda Jun 21, 2012

Contributor

Good catch, fixed.

+ def format_input_output(s)
+ s.empty? ? "(none)" : s
+ end
+
+ def format_commands(commands)
+ '"' + commands.map { |c| Shellwords.join(c) }.join(" | ") + '"'
end
end
@@ -219,10 +264,10 @@ def run(*args)
streamed = compute_streamed(options)
streams = build_streams(options, streamed)
commands = build_commands(args)
- logger = build_log_adapter(options)
+ recorder = build_recorder(options)
- log_commands(logger, commands)
- log_input(logger, options, streamed)
+ recorder.record_commands(commands)
+ recorder.record_stdin(streams[:stdin].string) unless streamed[:stdin]
pid, pipes = fork_commands(commands)
select_loop(streams, pipes)
@@ -231,8 +276,9 @@ def run(*args)
begin
check_errors(commands, status, streams, streamed)
ensure
- log_status(logger, status)
- log_output(logger, streams, streamed)
+ recorder.record_status(status)
+ recorder.record_stdout(streams[:stdout].string) unless streamed[:stdout]
+ recorder.record_stderr(streams[:stderr].string) unless streamed[:stderr]
end
build_result(streams, options)
@@ -282,8 +328,9 @@ def build_commands(args)
args.all? { |a| a.is_a?(Array) } ? args : [args]
end
- def build_log_adapter(options)
- LogAdapter.new(options[:logger])
+ def build_recorder(options)
+ klass = options[:logger] ? DefaultRecorder : NullRecorder
+ klass.new(options[:logger])
end
def fork_commands_recursive(commands, pipes)
@@ -431,42 +478,9 @@ def build_result(streams, options)
end
end
- # Logging
-
- def log_commands(logger, commands)
- logger.info "Executing #{format_commands(commands)}."
- end
-
- def log_input(logger, options, streamed)
- unless streamed[:stdin]
- logger.info "Standard input: #{format_input_output(options[:stdin])}"
- end
- end
-
- def log_status(logger, status)
- logger.send status.success? ? :info : :error,
- "Status: #{status.exitstatus}"
- end
-
- def log_output(logger, streams, streamed)
- unless streamed[:stdout]
- logger.info "Standard output: #{format_input_output(streams[:stdout].string)}"
- end
- unless streamed[:stderr]
- logger.send streams[:stderr].string.empty? ? :info : :error,
- "Error output: #{format_input_output(streams[:stderr].string)}"
- end
- end
-
- # Formatting
-
def format_commands(commands)
'"' + commands.map { |c| Shellwords.join(c) }.join(" | ") + '"'
end
-
- def format_input_output(s)
- s.empty? ? "(none)" : s
- end
end
self.default_options = {}
View
@@ -1,5 +1,98 @@
require "spec_helper"
+module Cheetah
+ describe DefaultRecorder do
+ before do
+ @logger = mock
+ @recorder = DefaultRecorder.new(@logger)
+ end
+
+ describe "record_commands" do
+ it "records execution of commands" do
+ @logger.should_receive(:info).with(
+ "Executing \"one foo bar baz | two foo bar baz | three foo bar baz\"."
+ )
+
+ @recorder.record_commands([
+ ["one", "foo", "bar", "baz"],
+ ["two", "foo", "bar", "baz"],
+ ["three", "foo", "bar", "baz"]
+ ])
+ end
+
+ it "escapes commands and their arguments" do
+ @logger.should_receive(:info).with(
+ "Executing \"we\\ \\!\\ ir\\ \\$d we\\ \\!\\ ir\\ \\$d we\\ \\!\\ ir\\ \\$d \\<\\|\\>\\$\"."
+ )
+
+ @recorder.record_commands([["we ! ir $d", "we ! ir $d", "we ! ir $d", "<|>$"]])
+ end
+ end
+
+ describe "record_stdin" do
+ it "records empty input" do
+ @logger.should_receive(:info).with("Standard input: (none)")
+
+ @recorder.record_stdin("")
+ end
+
+ it "records non-empty input" do
+ @logger.should_receive(:info).with("Standard input: input")
+
+ @recorder.record_stdin("input")
+ end
+ end
+
+ describe "record_status" do
+ it "records a success" do
+ @logger.should_receive(:info).with("Status: 0")
+
+ # I hate to mock Process::Status but it seems one can't create a new
+ # instance of it without actually running some process, which would be
+ # even worse.
+ @recorder.record_status(mock(:success? => true, :exitstatus => 0))
+ end
+
+ it "records a failure" do
+ @logger.should_receive(:error).with("Status: 1")
+
+ # I hate to mock Process::Status but it seems one can't create a new
+ # instance of it without actually running some process, which would be
+ # even worse.
+ @recorder.record_status(mock(:success? => false, :exitstatus => 1))
+ end
+ end
+
+ describe "record_stdout" do
+ it "records empty output" do
+ @logger.should_receive(:info).with("Standard output: (none)")
+
+ @recorder.record_stdout("")
+ end
+
+ it "records non-empty output" do
+ @logger.should_receive(:info).with("Standard output: output")
+
+ @recorder.record_stdout("output")
+ end
+ end
+
+ describe "record_stderr" do
+ it "records empty output" do
+ @logger.should_receive(:info).with("Error output: (none)")
+
+ @recorder.record_stderr("")
+ end
+
+ it "records non-empty output" do
+ @logger.should_receive(:error).with("Error output: output")
+
+ @recorder.record_stderr("output")
+ end
+ end
+ end
+end
+
describe Cheetah do
describe "run" do
# Fundamental question: To mock or not to mock the actual system interface?

0 comments on commit 54357bf

Please sign in to comment.