Skip to content
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

Reduce matcher interface #468

Merged
merged 1 commit into from Nov 7, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/flay.yml
@@ -1,3 +1,3 @@
---
threshold: 18
total_score: 1248
total_score: 1232
2 changes: 2 additions & 0 deletions lib/mutant.rb
Expand Up @@ -132,6 +132,7 @@ def self.ci?
require 'mutant/loader'
require 'mutant/context'
require 'mutant/context/scope'
require 'mutant/scope'
require 'mutant/subject'
require 'mutant/subject/method'
require 'mutant/subject/method/instance'
Expand All @@ -148,6 +149,7 @@ def self.ci?
require 'mutant/matcher/scope'
require 'mutant/matcher/filter'
require 'mutant/matcher/null'
require 'mutant/matcher/static'
require 'mutant/expression'
require 'mutant/expression/parser'
require 'mutant/expression/method'
Expand Down
44 changes: 36 additions & 8 deletions lib/mutant/env/bootstrap.rb
Expand Up @@ -4,10 +4,18 @@ class Env
class Bootstrap
include Adamantium::Flat, Concord::Public.new(:config, :cache), Procto.call(:env)

SEMANTICS_MESSAGE =
"Fix your lib to follow normal ruby semantics!\n" \
SEMANTICS_MESSAGE_FORMAT =
"%<message>s. Fix your lib to follow normal ruby semantics!\n" \
'{Module,Class}#name should return resolvable constant name as String or nil'.freeze

CLASS_NAME_RAISED_EXCEPTION =
'%<scope_class>s#name from: %<scope>s raised an error: %<exception>s'.freeze

CLASS_NAME_TYPE_MISMATCH_FORMAT =
'%<scope_class>s#name from: %<scope>s returned %<name>s'.freeze

private_constant(*constants(false))

# Scopes that are eligible for matching
#
# @return [Enumerable<Matcher::Scope>]
Expand Down Expand Up @@ -84,7 +92,12 @@ def env
def scope_name(scope)
scope.name
rescue => exception
warn("#{scope.class}#name from: #{scope.inspect} raised an error: #{exception.inspect}. #{SEMANTICS_MESSAGE}")
semantics_warning(
CLASS_NAME_RAISED_EXCEPTION,
scope_class: scope.class,
scope: scope,
exception: exception.inspect
)
nil
end

Expand All @@ -95,7 +108,7 @@ def scope_name(scope)
# @api private
def infect
config.includes.each(&$LOAD_PATH.method(:<<))
config.requires.each(&method(:require))
config.requires.each(&Kernel.method(:require))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious. Why did you add Kernel?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short: To be able to spec the global side effect this code should trigger.

Long:

When specifying requires, its IMO okay to rely on the fact Kernel#require will do the right thing when called. So its IMO fine to place message expectations to "specify" certain requires happen.

I could actually have setup a real file require it and observe a constant was added, but that would defeat the idea of a unit spec. A unit spec under execution should not mutate global state. (OT: Thats a nice way to try defining unit spec vs integration spec!)

Now when setting up message expectations (that are temporal monkey patches effectively) we need an object to target this monkey patches on.

2 thoughts arise from that:

  • The primary API of this class is Bootstrap.call that returns Env. No instance of Bootstrap is visible to the API client. The whole intention is to cause side effects and produce a "stable" environment object. Tests should only use the API of the class and do not "fuck around" with their implementation details. As I do not have access on an instance of Bootstrap via the API I do not have an object to set the message expectation up against.
  • If I'd even have an API where I could reach instances of Bootstrap setting up a message expectation on the object under test would be a very bad idea. Here I actually have to blame ruby because very important APIs with global side effects such as Kernel#require are available as private methods everywhere. I'd prefer it if require 'foo' would only work toplevel, or at class level, but not at instance level per default. This would reduce lots of confusion.

In summary: By changing the code to another construct, causing the same side effect. Its now more testable, as I can setup the message expectation on Kernel that is globally available.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another maybe more useful instance of the same pattern is available here: https://github.com/mbj/mutant/blob/master/bin/mutant#L13-L29

This is the call side for mutants self-zombification. It explicitly passes kernel into the zombifier API, for testing the zombifier I could implement a "fake Kernel" (that also does some message expectations, but not via rspec) to specify the behavior of Zombification.

Here is the test support class: https://github.com/mbj/mutant/blob/master/spec/support/ruby_vm.rb

This example is an instance of the same idea: When multiple APIs that produce the same effect exist (#require vs Kernel.require) use the one that is easier to unit test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interesting!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think next refactorings will pass in Kernel, $stderr, $stdout and other global friends explicitly into the mutant namespace. I had great success with that on the Mutant::Zombifier sub-project. So it'll help everywhere.

Each time I have to place a message expectation via rspec on a global constant something in my hart dies.

@integration = config.integration.new(config).setup
end

Expand All @@ -105,7 +118,7 @@ def infect
#
# @api private
def matched_subjects
Matcher::Compiler.call(self, config.matcher).to_a
Matcher::Compiler.call(config.matcher).call(self)
end

# Initialize matchable scopes
Expand All @@ -115,8 +128,8 @@ def matched_subjects
# @api private
def initialize_matchable_scopes
scopes = ObjectSpace.each_object(Module).each_with_object([]) do |scope, aggregate|
expression = expression(scope)
aggregate << Matcher::Scope.new(self, scope, expression) if expression
expression = expression(scope) || next
aggregate << Scope.new(scope, expression)
end

@matchable_scopes = scopes.sort_by { |scope| scope.expression.syntax }
Expand All @@ -137,12 +150,27 @@ def expression(scope)
name = scope_name(scope) or return

unless name.instance_of?(String)
warn("#{scope.class}#name from: #{scope.inspect} returned #{name.inspect}. #{SEMANTICS_MESSAGE}")
semantics_warning(
CLASS_NAME_TYPE_MISMATCH_FORMAT,
scope_class: scope.class,
scope: scope,
name: name
)
return
end

config.expression_parser.try_parse(name)
end

# Write a semantics warning
#
# @return [undefined]
#
# @api private
def semantics_warning(format, options)
message = format % options
warn(SEMANTICS_MESSAGE_FORMAT % { message: message })
end
end # Boostrap
end # Env
end # Mutant
8 changes: 3 additions & 5 deletions lib/mutant/expression/method.rb
Expand Up @@ -32,15 +32,13 @@ def syntax

# Matcher for expression
#
# @param [Env] env
#
# @return [Matcher]
#
# @api private
def matcher(env)
methods_matcher = MATCHERS.fetch(scope_symbol).new(env, scope)
def matcher
methods_matcher = MATCHERS.fetch(scope_symbol).new(scope)

Matcher::Filter.build(methods_matcher) { |subject| subject.expression.eql?(self) }
Matcher::Filter.new(methods_matcher, ->(subject) { subject.expression.eql?(self) })
end

private
Expand Down
6 changes: 2 additions & 4 deletions lib/mutant/expression/methods.rb
Expand Up @@ -26,13 +26,11 @@ def syntax

# Matcher on expression
#
# @param [Env] env
#
# @return [Matcher::Method]
#
# @api private
def matcher(env)
MATCHERS.fetch(scope_symbol).new(env, scope)
def matcher
MATCHERS.fetch(scope_symbol).new(scope)
end

# Length of match with other expression
Expand Down
12 changes: 4 additions & 8 deletions lib/mutant/expression/namespace.rb
Expand Up @@ -35,13 +35,11 @@ def syntax

# Matcher for expression
#
# @param [Env::Bootstrap] env
#
# @return [Matcher]
#
# @api private
def matcher(env)
Matcher::Namespace.new(env, self)
def matcher
Matcher::Namespace.new(self)
end

# Length of match with other expression
Expand Down Expand Up @@ -71,13 +69,11 @@ class Exact < self

# Matcher matcher on expression
#
# @param [Env::Bootstrap] env
#
# @return [Matcher]
#
# @api private
def matcher(env)
Matcher::Scope.new(env, Object.const_get(scope_name), self)
def matcher
Matcher::Scope.new(Object.const_get(scope_name))
end

# Syntax for expression
Expand Down
21 changes: 3 additions & 18 deletions lib/mutant/matcher.rb
@@ -1,30 +1,15 @@
module Mutant
# Abstract matcher to find subjects to mutate
class Matcher
include Adamantium::Flat, Enumerable, AbstractType
include Adamantium::Flat, AbstractType

# Default matcher build implementation
# Call matcher
#
# @param [Env] env
# @param [Object] input
#
# @return [undefined]
#
# @api private
def self.build(*arguments)
new(*arguments)
end

# Enumerate subjects
#
# @return [self]
# if block given
#
# @return [Enumerable<Subject>]
# otherwise
#
# @api private
abstract_method :each
abstract_method :call

end # Matcher
end # Mutant
20 changes: 7 additions & 13 deletions lib/mutant/matcher/chain.rb
@@ -1,26 +1,20 @@
module Mutant
class Matcher
# A chain of matchers
# Matcher chaining results of other matchers together
class Chain < self
include Concord.new(:matchers)

# Enumerate subjects
# Call matcher
#
# @return [Enumerator<Subject]
# if no block given
# @param [Env] env
#
# @return [self]
# otherwise
# @return [Enumerable<Subject>]
#
# @api private
def each(&block)
return to_enum unless block_given?

matchers.each do |matcher|
matcher.each(&block)
def call(env)
matchers.flat_map do |matcher|
matcher.call(env)
end

self
end

end # Chain
Expand Down
15 changes: 2 additions & 13 deletions lib/mutant/matcher/compiler.rb
Expand Up @@ -3,7 +3,7 @@ class Matcher

# Compiler for complex matchers
class Compiler
include Concord.new(:env, :config), AST::Sexp, Procto.call(:result)
include Concord.new(:config), AST::Sexp, Procto.call(:result)

# Generated matcher
#
Expand All @@ -12,7 +12,7 @@ class Compiler
# @api private
def result
Filter.new(
Chain.build(config.match_expressions.map(&method(:matcher))),
Chain.new(config.match_expressions.map(&:matcher)),
Morpher::Evaluator::Predicate::Boolean::And.new(
[
ignored_subjects,
Expand Down Expand Up @@ -61,17 +61,6 @@ def filtered_subjects
Morpher::Evaluator::Predicate::Boolean::And.new(config.subject_filters)
end

# Matcher for expression on env
#
# @param [Mutant::Expression] expression
#
# @return [Matcher]
#
# @api private
def matcher(expression)
expression.matcher(env)
end

end # Compiler
end # Matcher
end # Mutant
25 changes: 6 additions & 19 deletions lib/mutant/matcher/filter.rb
Expand Up @@ -4,30 +4,17 @@ class Matcher
class Filter < self
include Concord.new(:matcher, :predicate)

# New matcher
#
# @param [Matcher] matcher
#
# @return [Matcher]
#
# @api private
def self.build(matcher, &predicate)
new(matcher, predicate)
end

# Enumerate matches
#
# @return [self]
# if block given
# @param [Env] env
#
# @return [Enumerator<Subject>]
# otherwise
# @return [Enumerable<Subject>]
#
# @api private
def each(&block)
return to_enum unless block_given?
matcher.select(&predicate.method(:call)).each(&block)
self
def call(env)
matcher
.call(env)
.select(&predicate.method(:call))
end

end # Filter
Expand Down