Improve mutation coverage

[backus]

Resolves mutations uncovered by #27

[backus]

@dkubb how would you tackle killing a mutation like this?

@@ -1,4 +1,5 @@
 def self.measure_string(string, config = Config.new)
+  config = Config.new
   Processor.new(config).process_string(string)
 end

I can think of a handful of ways that would kill it. For example, passing in a custom configuration and saying something like expect(custom_config).to receive(:for_rule).at_least(10).times but this sort of solution doesn't seem good enough really.

[dkubb]

how would you tackle killing a mutation like this?

@@ -1,4 +1,5 @@
 def self.measure_string(string, config = Config.new)
+  config = Config.new
   Processor.new(config).process_string(string)
 end 

@backus A method that receives an optional argument has at least two code branches that need testing:

1. The method called without the optional argument. This should test the default behaviour and make sure it is valid.
2. The method called with an optional argument. This should test to make sure the behaviour of the method with the special optional argument.

I can think of a handful of ways that would kill it. For example, passing in a custom configuration and saying something like expect(custom_config).to receive(:for_rule).at_least(10).times but this sort of solution doesn't seem good enough really.

I think it depends on the contract of the method under test. There's definitely a point where you've asserted so much of the method's behaviour that you've basically coded something that is directly based on the implementation.

OTOH when you pass an argument to a method, the method has certain expectations of the argument's interface, and I think it's ok to assert on that behaviour. What the method does, aside from the interfaces it uses in the arguments, should mostly be thought of as a black box.

[backus]

in c6d9227c1cc086ee80681dfd1a781bfdc530e1da, f4a7f27bcd2fef7fb49445f33485c39125f78e9a, and 26af407dac629e582fd6c023a31c6d7caa559990 I removed files that specified #initialize. Doing so improved the mutation coverage since mutant then selected every test under Yardstick::Measurement and Yardstick::RuleConfig instead.

[backus]

@dkubb The following mutation

@@ -1,5 +1,5 @@
 def self.parse_paths(paths)
-  YARD.parse(paths, [], YARD::Logger::ERROR)
+  YARD.parse(paths, [], Logger::ERROR)
   documents
 end

depends on YARD autoloading. If the line is changed to either of the following

YARD.parse(paths, [], Logger::ERROR) # => "uninitialized constant Yardstick::Parser::Logger"

YARD.parse(paths, [], ::Logger::ERROR) # => "NameError: uninitialized constant Logger"

then I occasionally encounter their respective errors if YARD::Logger is not referenced (triggering autoload) before that line is encountered.

• I tried using

  before do
  hide_const('Logger')

  but this unfortunately keeps ::Logger hidden even after the logging library is required.

• I tried using removing the Logger constant like so

  before do
  # # kill YARD autoloading mutation
  Object.send(:remove_const, 'Logger') if Object.const_defined?(:Logger)

  but $LOADED_FEATURES still includes 'logger.rb' so, unless I want to modify that, I end up with an order dependency in the tests.

It seems like the best option would just be adding require 'yard/logging' to lib/yardstick.rb. Any thoughts on other solutions?

[dkubb]

It seems like the best option would just be adding require 'yard/logging' to lib/yardstick.rb. Any thoughts on other solutions?

I dislike relying on autoloading, so I'm fine with adding an explicit require statement.

[backus]

@dkubb @mbj regarding https://github.com/dkubb/yardstick/commit/aa528c65db3bfb52d8fe4d46e26e6aef5fb68a35 introducing the two select calls in the following snippet:

def self.method_objects
  YARD::Registry.all(:method).select(&:file).select(&:line).sort_by do |method_object|
    [method_object.file, method_object.line]
  end
ensure
  YARD::Registry.clear
end

I can't find a case where method_object.file would not be nil but method_object.line would. Am I missing something here or would it be safe to remove select(&:line)?

edit: I made this change in 92c814de302d0df1b6c10fb8fd59379a0d06723a. If I am missing an edge case then I can just delete this change.

[backus]

@dkubb 100% coverage! :smiley: Ready for review

edit: I do have one request assuming you have desired changes. Could we change this repo to use CircleCI? Travis does not play well with git push-each and it makes quick history rewrites take hours with a lot of additional manual effort.

[dkubb]

@backus ok I finished a pass on the code.

[backus]

https://github.com/backus/yardstick/commit/2b682eb6fd26cfe577e02d63dd7515f665ea3dd6 changed the number of mutations possible for that state of the code which ripples down affecting all later commits. Will have time to fix this tomorrow

[backus]

@dkubb all commits are green again. Ready for another pass

[dkubb]

@backus thanks so much for this! merged.

[backus]

:smile: :balloon: