Skip to content

Commit

Permalink
Add CC::Logger
Browse files Browse the repository at this point in the history
We have this in other engines and it allows for better handling of debug
vs info/warn engine logging.
  • Loading branch information
dblandin committed Aug 30, 2017
1 parent 4ca4466 commit a55e616
Show file tree
Hide file tree
Showing 17 changed files with 61 additions and 110 deletions.
8 changes: 8 additions & 0 deletions bin/duplication
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# frozen_string_literal: true

$:.unshift(File.expand_path(File.join(File.dirname(__FILE__), "../lib")))
require "cc/logger"
require "cc/engine/duplication"

engine_config =
Expand All @@ -11,6 +12,13 @@ engine_config =
{}
end

CC.logger.level =
if config["debug"]

This comment has been minimized.

Copy link
@winniehell

winniehell Aug 30, 2017

@dblandin should this be engine_config? we are getting

/usr/src/app/bin/duplication:16:in `<main>': undefined local variable or method `config' for main:Object (NameError)

(https://gitlab.com/gitlab-org/gitlab-ce/-/jobs/30805895)

This comment has been minimized.

Copy link
@winniehell

winniehell Aug 30, 2017

I have created #212 now.

This comment has been minimized.

Copy link
@dblandin

dblandin Aug 30, 2017

Author Contributor

Ahh, yes. Good catch. Will open up a PR. Thanks!

::Logger::DEBUG
else
::Logger::INFO
end

directory = ARGV[0] || "/code"

CC::Engine::Duplication.new(
Expand Down
8 changes: 5 additions & 3 deletions lib/cc/engine/analyzers/analyzer_base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,17 @@ def initialize(engine_config:)

def run(file)
if (skip_reason = skip?(file))
$stderr.puts("Skipping file #{file} because #{skip_reason}")
CC.logger.info("Skipping file #{file} because #{skip_reason}")
nil
else
process_file(file)
end
rescue => ex
if RESCUABLE_ERRORS.map { |klass| ex.instance_of?(klass) }.include?(true)
$stderr.puts("Skipping file #{file} due to exception (#{ex.class}): #{ex.message}\n#{ex.backtrace.join("\n")}")
CC.logger.info("Skipping file #{file} due to exception (#{ex.class}): #{ex.message}\n#{ex.backtrace.join("\n")}")
nil
else
$stderr.puts("#{ex.class} error occurred processing file #{file}: aborting.")
CC.logger.info("#{ex.class} error occurred processing file #{file}: aborting.")
raise ex
end
end
Expand Down
4 changes: 0 additions & 4 deletions lib/cc/engine/analyzers/engine_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ def initialize(hash)
@config = normalize(hash)
end

def debug?
config.fetch("config", {}).fetch("debug", "false").to_s.casecmp("true").zero?
end

def include_paths
config.fetch("include_paths", ["./"])
end
Expand Down
2 changes: 1 addition & 1 deletion lib/cc/engine/analyzers/javascript/parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def parse

self
rescue Timeout::Error
warn "TIMEOUT parsing #{filename}. Skipping."
CC.logger.warn("TIMEOUT parsing #{filename}. Skipping.")
end

private
Expand Down
2 changes: 1 addition & 1 deletion lib/cc/engine/analyzers/php/parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def parse

self
rescue Timeout::Error
warn "TIMEOUT parsing #{filename}. Skipping."
CC.logger.warn("TIMEOUT parsing #{filename}. Skipping.")
end

private
Expand Down
2 changes: 1 addition & 1 deletion lib/cc/engine/analyzers/python/parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def parse

self
rescue Timeout::Error
warn "TIMEOUT parsing #{filename}. Skipping."
CC.logger.warn("TIMEOUT parsing #{filename}. Skipping.")
end

private
Expand Down
30 changes: 9 additions & 21 deletions lib/cc/engine/analyzers/reporter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@ def initialize(engine_config, language_strategy, io)
end

def run
debug("Processing #{language_strategy.files.count} #{lang} files concurrency=#{engine_config.concurrency}")
CC.logger.debug("Processing #{language_strategy.files.count} #{lang} files concurrency=#{engine_config.concurrency}")

process_files

if engine_config.dump_ast?
dump_ast
else
report
debug("Reported #{reports.size} violations...")
CC.logger.debug("Reported #{reports.size} violations...")
end
end

Expand All @@ -39,11 +39,10 @@ def dump_ast

return if issues.empty?

debug "Sexps for issues:"
debug
CC.logger.debug("Sexps for issues:")

issues.each_with_index do |issue, idx1|
debug(
CC.logger.debug(
format(
"#%2d) %s#%d mass=%d:",
idx1 + 1,
Expand All @@ -52,20 +51,15 @@ def dump_ast
issue.mass,
),
)
debug

locs = issue.locations.map.with_index do |loc, idx2|
format("# %d.%d) %s:%s", idx1 + 1, idx2 + 1, loc.file, loc.line)
end

locs.zip(flay.hashes[issue.structural_hash]).each do |loc, sexp|
debug loc
debug
debug sexp.pretty_inspect
debug
CC.logger.debug(loc)
CC.logger.debug(sexp.pretty_inspect)
end

debug
end
end

Expand All @@ -78,7 +72,7 @@ def process_files
processed_files_count = Concurrent::AtomicFixnum.new

pool.run do |file|
debug("Processing #{lang} file: #{file}")
CC.logger.debug("Processing #{lang} file: #{file}")

sexp = language_strategy.run(file)

Expand All @@ -89,7 +83,7 @@ def process_files

pool.join

debug("Processed #{processed_files_count.value} #{lang} files")
CC.logger.debug("Processed #{processed_files_count.value} #{lang} files")
end

def lang
Expand All @@ -102,7 +96,7 @@ def report

violations.each do |violation|
next if (violation.occurrences + 1) < language_strategy.count_threshold
debug("Violation name=#{violation.report_name} mass=#{violation.mass}")
CC.logger.debug("Violation name=#{violation.report_name} mass=#{violation.mass}")

unless reports.include?(violation.report_name)
reports.add(violation.report_name)
Expand Down Expand Up @@ -140,12 +134,6 @@ def flay_options

CCFlay.default_options.merge changes
end

def debug(message = "")
IO_M.synchronize do
$stderr.puts(message) if engine_config.debug?
end
end
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/cc/engine/analyzers/ruby/main.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class Main < CC::Engine::Analyzers::Base
def process_file(file)
RubyParser.new.process(File.binread(file), file, TIMEOUT)
rescue Timeout::Error
warn "TIMEOUT parsing #{file}. Skipping."
CC.logger.warn("TIMEOUT parsing #{file}. Skipping.")
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/cc/engine/duplication.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def languages

if languages.empty?
message = "Config Error: Unable to run the duplication engine without any languages enabled."
$stderr.puts message
CC.logger.info(message)
raise EmptyLanguagesError, message
else
languages
Expand Down
9 changes: 9 additions & 0 deletions lib/cc/logger.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# frozen_string_literal: true

require "logger"

module CC
def self.logger
@logger ||= ::Logger.new(STDERR)
end
end
32 changes: 0 additions & 32 deletions spec/cc/engine/analyzers/engine_config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -212,38 +212,6 @@
end
end

describe "debug" do
it "passes through booleans" do
engine_config = CC::Engine::Analyzers::EngineConfig.new({
"config" => {
"debug" => true,
},
})

expect(engine_config.debug?).to eq(true)
end

it "coerces 'true' to true" do
engine_config = CC::Engine::Analyzers::EngineConfig.new({
"config" => {
"debug" => "true",
},
})

expect(engine_config.debug?).to eq(true)
end

it "coerces 'false' to false" do
engine_config = CC::Engine::Analyzers::EngineConfig.new({
"config" => {
"debug" => "false",
},
})

expect(engine_config.debug?).to eq(false)
end
end

describe "#patterns_for" do
it "returns patterns for specified language" do
engine_config = CC::Engine::Analyzers::EngineConfig.new({
Expand Down
14 changes: 7 additions & 7 deletions spec/cc/engine/analyzers/javascript/main_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,26 +74,26 @@
});
EOJS

expect { run_engine(engine_conf) }.not_to output(/Skipping file/).to_stderr

expect(CC.logger).not_to receive(:info).with(/Skipping file/)
run_engine(engine_conf)
end

it "skips unparsable files" do
create_source_file("foo.js", <<-EOJS)
function () { do(); // missing closing brace
EOJS

expect {
expect(run_engine(engine_conf)).to eq("")
}.to output(/Skipping file/).to_stderr
expect(CC.logger).to receive(:info).with(/Skipping file/)
expect(run_engine(engine_conf)).to eq("")
end

it "skips minified files" do
path = fixture_path("huge_js_file.js")
create_source_file("foo.js", File.read(path))

expect {
expect(run_engine(engine_conf)).to eq("")
}.to output(/Skipping file/).to_stderr
expect(CC.logger).to receive(:info).with(/Skipping file/)
expect(run_engine(engine_conf)).to eq("")
end
end

Expand Down
5 changes: 2 additions & 3 deletions spec/cc/engine/analyzers/php/main_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,8 @@
<?php blorb &; "fee
EOPHP

expect {
expect(run_engine(engine_conf)).to eq("")
}.to output(/Skipping file/).to_stderr
expect(CC.logger).to receive(:info).with(/Skipping file/)
expect(run_engine(engine_conf)).to eq("")
end

it "can parse php 7 code" do
Expand Down
5 changes: 2 additions & 3 deletions spec/cc/engine/analyzers/python/main_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,8 @@ def c(thing: str):
---
EOPY

expect {
expect(run_engine(engine_conf)).to eq("")
}.to output(/Skipping file/).to_stderr
expect(CC.logger).to receive(:info).with(/Skipping file/)
expect(run_engine(engine_conf)).to eq("")
end
end

Expand Down
10 changes: 4 additions & 6 deletions spec/cc/engine/analyzers/ruby/main_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,8 @@ def self.sub_degrees(str)
EORUBY

pending "Potential lexing bug. Ask customer to remove escaping."
expect {
expect(run_engine(engine_conf)).to eq("")
}.to output(/Skipping file/).to_stderr
expect(CC.logger).to receive(:info).with(/Skipping file/)
expect(run_engine(engine_conf)).to eq("")
end

it "calculates locations correctly for conditional statements" do
Expand Down Expand Up @@ -145,9 +144,8 @@ def self.from_remediation_amount(amount)
---
EORUBY

expect {
expect(run_engine(engine_conf)).to eq("")
}.to output(/Skipping file/).to_stderr
expect(CC.logger).to receive(:info).with(/Skipping file/)
expect(run_engine(engine_conf)).to eq("")
end

it "does not see hashes as similar" do
Expand Down
7 changes: 3 additions & 4 deletions spec/cc/engine/duplication_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,9 @@
original_directory = Dir.pwd
engine = CC::Engine::Duplication.new(directory: @directory, engine_config: {}, io: StringIO.new)

expected_output = "Config Error: Unable to run the duplication engine without any languages enabled.\n"
expect {
expect { engine.run }.to raise_error(CC::Engine::Duplication::EmptyLanguagesError)
}.to output(expected_output).to_stderr
expected_output = "Config Error: Unable to run the duplication engine without any languages enabled."
expect(CC.logger).to receive(:info).with(expected_output)
expect { engine.run }.to raise_error(CC::Engine::Duplication::EmptyLanguagesError)

Dir.chdir(original_directory)
end
Expand Down
29 changes: 7 additions & 22 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
require 'bundler/setup'
require 'flay'
require 'tmpdir'

require "bundler/setup"
require "flay"
require "tmpdir"
require "pry"
Pry.config.pager = false
Pry.config.color = false

require "cc/logger"

Dir[File.dirname(__FILE__) + "/support/**/*.rb"].each {|f| require f }

RSpec.configure do |config|
Expand All @@ -27,24 +28,6 @@
end
end

class DummyStderr
def write(*)
end

def method_missing(*)
end
end

unless ENV["ENGINE_DEBUG"]
config.before(:each) do
$stderr = DummyStderr.new
end

config.after(:each) do
$stderr = STDERR
end
end

config.order = :random
config.disable_monkey_patching!

Expand All @@ -53,3 +36,5 @@ def method_missing(*)
config.alias_example_to :pit, pending: true
config.run_all_when_everything_filtered = true
end

CC.logger.level = ::Logger::ERROR

0 comments on commit a55e616

Please sign in to comment.