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

Introduce Minitest integration #330

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@kbrock
Copy link
Contributor

kbrock commented May 14, 2015

Hi

This is working for me, but looking for some input.

  1. Suggestions on how to handle lib and test requires? I though the mutant -I test -I lib would have loaded them into the LOAD_PATH, but they were not set. Should I load these from config? Is that available?
  2. Suggestions on loading in the tests? (this seems to be the way rake testtask does it)
  3. What belongs in run vs setup?
  4. How much output do you want from minitest while running tests?
  5. Do you want to start imposing "describe" type tests?
  6. Do you want to support "minitest spec tests too?"

cleanup:

  1. need to squash
  2. need to remove rakefile stuff
  3. Want to include attribution, but this code has reworked so many times by different people that I'm not sure which git commits use as my base. And a majority of the lines have been changed. You have a preference for a bunch of commits vs clean git history?

Thanks

@kbrock kbrock changed the title Minitest integ Introduce Minitest integration May 14, 2015

@kbrock kbrock changed the title Introduce Minitest integration [WIP] Introduce Minitest integration May 14, 2015

def initialize(class_name, test_method)
@class_name = class_name
@test_method = test_method
end

This comment has been minimized.

@mbj

mbj May 14, 2015

Owner

Mutant depends on concord, which allows to combine this:

        attr_accessor :class_name
        attr_accessor :test_method

        def initialize(class_name, test_method)
          @class_name = class_name
          @test_method = test_method
        end

Into:

include Concord::Public.new(:class_name, :test_method)

Which is shorter to write/read and requires less unit testing as concord is mutation covered already and the injected semantics are trustwrothy because "trust your dependencies".

This comment has been minimized.

@mbj

mbj May 14, 2015

Owner

#class_name, and #test_method do not have public call sides, so you can use include Concord.new(:class_name, :test_method) to not create the public interface.

@test_method = test_method
end

def clazz

This comment has been minimized.

@mbj

mbj May 14, 2015

Owner

I think its the ruby convention to name methods returning instances of Class #klass.

This comment has been minimized.

@mbj

mbj May 14, 2015

Owner

Also there is no public call side for this method, we want to make it private for that reason.


def method_name
test_method[5..-1]
end

This comment has been minimized.

@mbj

mbj May 14, 2015

Owner

I think this method should be memoized via Adamantium to create a truly idempotent getter #method_name.

This comment has been minimized.

@kbrock

kbrock May 15, 2015

Contributor

done

end

ALL = Mutant::Expression.parse('*')
EXPRESSION_DELIMITER = ' '.freeze

This comment has been minimized.

@mbj

mbj May 14, 2015

Owner

These constants are duplicated in the rspec integration, lets move them to the base class Mutant::Integration.

def make_reporter(output)
options = { io: output }
reporter = ::Minitest::CompositeReporter.new
reporter << ::Minitest::SummaryReporter.new(options[:io], options)

This comment has been minimized.

@mbj

mbj May 14, 2015

Owner

Instead to access the hash key :io, lets just pass in the lvar output as the first argument.

options = { io: output }
reporter = ::Minitest::CompositeReporter.new
reporter << ::Minitest::SummaryReporter.new(options[:io], options)
reporter << ::Minitest::ProgressReporter.new(options[:io], options)

This comment has been minimized.

@mbj
# summary reporter detects failures
# progress reporter prints dots
# could include option verbose if desired
def make_reporter(output)

This comment has been minimized.

@mbj

mbj May 14, 2015

Owner

This method has no public call side, so lets make it private.

This comment has been minimized.

@kbrock

kbrock May 15, 2015

Contributor

done

examples = tests.map(&all_tests_index.method(:fetch)).to_set
start = Time.now

examples.detect { |test| !test.runs_successfully?(@reporter) }

This comment has been minimized.

@mbj

mbj May 14, 2015

Owner

The return value of Enumerable#detect is unused here. We should use Enumerable#each to reduce the amount of unused semantics being executed.

This comment has been minimized.

@kbrock

kbrock May 15, 2015

Contributor

detect bails on the first error.

I used this to make rubocop happy.

I'll put in other changes and see how it goes.

This comment has been minimized.

@mbj

mbj May 15, 2015

Owner

Ahh I see. This is more side effect full than I thought.

def setup
$LOAD_PATH << 'test' unless $LOAD_PATH.include?('test')
$LOAD_PATH << 'lib' unless $LOAD_PATH.include?('lib')
Dir.glob('./test/**/*_test.rb').each { |f| require f }

This comment has been minimized.

@mbj

mbj May 14, 2015

Owner

Instead of the explicit block being passed to Enumerable#each I think we should go for the shorter version each(&method(:require)).

$LOAD_PATH << 'test' unless $LOAD_PATH.include?('test')
$LOAD_PATH << 'lib' unless $LOAD_PATH.include?('lib')
Dir.glob('./test/**/*_test.rb').each { |f| require f }
Dir.glob('./test/**/test_*.rb').each { |f| require f }

This comment has been minimized.

@mbj
end

def clazz
Object.const_get(class_name)

This comment has been minimized.

@mbj

mbj May 14, 2015

Owner

Are you sure this works on nested classes? I think you better want to use Mutant.constant_lookup which handles nested classes correctly.

@mbj

This comment has been minimized.

Copy link
Owner

mbj commented May 14, 2015

@kbrock Cool. I left some quick code only comments, my OSS time is very limited will look into this ASAP!

@kbrock kbrock force-pushed the kbrock:minitest branch from f45f070 to 668c8bc May 15, 2015

@kbrock

This comment has been minimized.

Copy link
Contributor

kbrock commented May 15, 2015

Darn, introduced a failure somewhere.

There are a few methods that are common that I could move up. not sure if it is good:

  1. all_tests
  2. all_tests_index
  3. parse_example
    or maybe: test_from_description("minitest", index, full_description)

@kbrock kbrock force-pushed the kbrock:minitest branch from 39717af to 544e0b0 May 16, 2015

@dkubb

This comment has been minimized.

Copy link
Collaborator

dkubb commented May 17, 2015

@kbrock One thing I often do to isolate which commit introduced an error is to push each commit individually using something like https://github.com/dkubb/git-tools/blob/master/git-push-each

When each commit is pushed individually, CircleCI will run the tests on each one. The first one to fail introduced the error so you can at least narrow down where a fix should be applied.

If you comment out the "^$remote/$branch" part on line 13 you should be able to re-push each commit in this branch.

@dkubb

This comment has been minimized.

Copy link
Collaborator

dkubb commented May 17, 2015

@mbj why is travis-ci being used for tests? From what I saw in other branches it was failing yet you still merged. What's the point in having it around if it's not a blocker for merging?

@dkubb

This comment has been minimized.

Copy link
Collaborator

dkubb commented May 17, 2015

@kbrock also if you'd like a hand with this branch, feel free to add me as a committer. I could fix the merge conflict and push each commit if you want.

end

def method_name
test_method[5..-1]

This comment has been minimized.

@dkubb

dkubb May 17, 2015

Collaborator

Would test_method.drop(5) work just as well here?

This comment has been minimized.

@kbrock

kbrock May 18, 2015

Contributor

I like the look of that, but I tried "abcde".drop(2) in ruby 2.1 and it failed.
Is that an active support thing?

This comment has been minimized.

@dkubb

dkubb May 18, 2015

Collaborator

What is test_method ? If it's a String then your way is probably better since String is not Enumerable. I was thinking it was an Array or something, in which case #drop is better.

Aside: I'm not really sure why they decided String shouldn't be Enumerable. I find myself wanting to treat it as a list of "things" and apply Enumerable methods on a semi-regular basis.

end

def setup
$LOAD_PATH << 'test' unless $LOAD_PATH.include?('test')

This comment has been minimized.

@dkubb

dkubb May 17, 2015

Collaborator

What about:

$LOAD_PATH |= %w[test lib]

This comment has been minimized.

@kbrock

kbrock May 18, 2015

Contributor

thanks. I really don't like messing with the load path here.

Actually, I don't like anything that I did in setup.

Also not sure when setup should be called

This comment has been minimized.

@kbrock

kbrock May 18, 2015

Contributor

$LOAD_PATH is readonly - won't work.

I could do:

%w[test lib].each { |dir| $LOAD_PATH << dir unless $LOAD_PATH.include?(dir) }

This comment has been minimized.

@dkubb

dkubb May 18, 2015

Collaborator

I guess that's slightly better since it removes duplication.

Is there any harm in doing $LOAD_PATH.concat(%w[test lib])? If those directories do happen to exist in the load path, appending them at the end isn't going to make a difference.

This comment has been minimized.

@mbj

mbj May 18, 2015

Owner

Also not sure when setup should be called

Mutant needs precise control on when it "infects" the VM under its control with 3rd party code. For that reason there is the distinction between #initialize and #setup.

This distinction might be more relevant for rspec. I'll look into it once I have the time to test this branch.

examples = tests.map(&all_tests_index.method(:fetch)).to_set
start = Time.now

examples.each { |test| break unless test.run_passes?(@reporter) }

This comment has been minimized.

@dkubb

dkubb May 17, 2015

Collaborator

It's debateable, but I wonder if this would be simpler:

examples.all? { |test| test.run_passes?(@reporter) }

Enumerable#all? still short circuits if one of the tests returns false.

This comment has been minimized.

@mbj

mbj May 17, 2015

Owner

I think thats better, as it is overall less AST.

@mbj

This comment has been minimized.

Copy link
Owner

mbj commented May 17, 2015

@mbj why is travis-ci being used for tests? From what I saw in other branches it was failing yet you still merged. What's the point in having it around if it's not a blocker for merging?

I only use circle these days for mutant. As travis is so slow. Also it is a way more indeterministic.

Before each release I typically make travis pass for all matrix elements, so yeah it has some value. I wounder if I can reduce travis usage only for master builds.

@dkubb

This comment has been minimized.

Copy link
Collaborator

dkubb commented May 17, 2015

Before each release I typically make travis pass for all matrix element

@mbj my guess is that CircleCI runs a different set of checks than TravisCI. If they are put in sync I would guess most of the time if one passes the other will pass too (... eventually, in the case of TravisCI).

I wounder if I can reduce travis usage only for master builds.

I'd support just having it run on master only. I know it can be done since we previously restricted DM to run on commits to release-* branches. The full matrix test only makes sense on releasable code, and CircleCI does a better job quickly testing feature branches.

OT but does anyone know if TravisCI is overprovisioned or something? It doesn't look very good for their service when people testing it out have to wait so much longer for results compared to CircleCI. I would much prefer if they were closer in performance, because I think the competition is beneficial to everyone, but at the moment it's no contest.

@mbj

This comment has been minimized.

Copy link
Owner

mbj commented May 17, 2015

I'd support just having it run on master only. I know it can be done since we previously restricted DM to run on commits to release-* branches. The full matrix test only makes sense on releasable code, and CircleCI does a better job quickly testing feature branches.

I actually considered to use a payed plan for my most popular OSS projects. To get full speedy builds with a matrix. There are Circle/Travis competitors that might have this feature (matrix + fast) already.

@kbrock

This comment has been minimized.

Copy link
Contributor

kbrock commented May 18, 2015

@dkubb looks like I was not calling setup in the tests.

@mbj I'm confused about the contract with Integration.setup(name). I expect that code to call setup on the class the first time it is loaded. Thoughts?

@kbrock kbrock force-pushed the kbrock:minitest branch 2 times, most recently from 8520702 to 033b7d3 May 18, 2015

@kbrock

This comment has been minimized.

Copy link
Contributor

kbrock commented May 18, 2015

squashed.

Ideas why I need to add test and lib to $LOAD_PATH?
Shouldn't calling mutant with -I test -I list do taht for me?

@mperham

This comment has been minimized.

Copy link

mperham commented Jul 7, 2015

@backus

This comment has been minimized.

Copy link
Contributor

backus commented Jul 7, 2015

To get a better oversight on minitest spec styles (to find a way to suppor them all), could the minitest users listening to this please name projects that qualify for:

  • Small
  • Very high test coverage (ideally 100% mutation coverage)
  • Uses an ideomatic minitest style consistently
  • Ideally we have coverage for all major minitest styles.

@zenspider might have some good input here since he is the largest contributor listed on github for minitest, ruby2ruby, and heckle.

@mbj

This comment has been minimized.

Copy link
Owner

mbj commented Jul 7, 2015

You tell me. https://github.com/mperham/sidekiq/tree/master/test

I've no idea, as I never did much with minitest. Thats my problem here.

I do not know what to expect. From reading peoples responses there seem to be at least two styles:

  • Test prefixed classes with test_ prefixed methods
  • Something else with an rspec-a-like syntax, that might generate the above internally, but potentially generates method names that can only be meta-programmed but not be literally present in a ruby source file.

My problem is: Is the listing from above complete, or not?

@mperham

This comment has been minimized.

Copy link

mperham commented Jul 7, 2015

Afaik, your list is complete.

Traditional:
https://github.com/mperham/sidekiq/blob/master/test/test_web_helpers.rb
Spec: https://github.com/mperham/sidekiq/blob/master/test/test_manager.rb

Sidekiq overwhelmingly uses spec but there's one or two uses of traditional.

On Tue, Jul 7, 2015 at 3:24 PM, Markus Schirp notifications@github.com
wrote:

You tell me. https://github.com/mperham/sidekiq/tree/master/test

I've no idea, as I never did much with minitest. Thats my problem here.

I do not know what to expect. From reading peoples responses there seem
to be at least two styles:

  • Test suffixed classes with test_ prefixed methods
  • Something else with an rspec-a-like syntax, that might generate the
    above internally, but potentially generates method names that can only be
    meta-programmed but not be literally present in a ruby source file.

My problem is: Is the listing from above complete, or not?


Reply to this email directly or view it on GitHub
#330 (comment).

@mbj

This comment has been minimized.

Copy link
Owner

mbj commented Jul 7, 2015

Afaik, your list is complete.

Thx that helps a lot. Can I assume there is an 1:1 mapping between Test prefixed class/module with one in the tested code?

So Foo is in the traditional style always tested via TestFoo ?

@mperham

This comment has been minimized.

Copy link

mperham commented Jul 7, 2015

Yeah, I'd think so. Note I use TestManager, not TestSidekiqManager, to test Sidekiq::Manager so it's not directly mappable.

@mbj

This comment has been minimized.

Copy link
Owner

mbj commented Jul 7, 2015

Better question: How could mutant infer that this test:

https://github.com/mperham/sidekiq/blob/master/test/test_manager.rb

Is meaning to specify behavior of this class:

https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/manager.rb

I actually could trace normal test execution to determine this. But I do not like this for various reasons as it has a fundamental downside I typically refer to as "implicit coverage". I plan to support such "implicit" coverage as a side effect of another planned feature.

But ideally we find a way to explicitly specify this relationship. Does minitest in "spec" style have support for metadata. So could I write a test like this:

class TestManager < Sidekiq::Test
  describe 'manager', specifies: Sidekiq::Manager do
    ...

?

@mperham

This comment has been minimized.

Copy link

mperham commented Jul 7, 2015

I don't think you can. I can see how you could rely on rspec's describe(class) for that but I don't know if minitest has the equivalent.

@mperham

This comment has been minimized.

Copy link

mperham commented Jul 7, 2015

I'd be happy to mark up the Sidekiq test suite to support Mutant if you gave minitest users an annotation to use, e.g.

class TestManager < Minitest::Test
  exercises Sidekiq::Manager
@mbj

This comment has been minimized.

Copy link
Owner

mbj commented Jul 7, 2015

I don't think you can. I can see how you could rely on rspec's describe(class) for that but I don't know if minitest has the equivalent.

And thats the key point. This explicit mapping is easy for rspec. And it has a big leverage for execution speed and coverage correctness.

Minitest integration always failed in the past for the lack of this specific mapping metadata. Unless I get the time to do the tracing based selection I see no way how we could do a good rspec equivalent mutant integration with minitest - unless we add this metadata to minitest.

And I'm not an minitest user both OSS and commercial - so there was not enough incentive to actually do one of the proposed solutions. Just running all tests per mutation is not an option. Both because implicit coverage and runtime.

@mbj

This comment has been minimized.

Copy link
Owner

mbj commented Jul 7, 2015

I'd be happy to mark up the Sidekiq test suite to support Mutant if you gave minitest users an annotation to use, e.g.

Yeah, these are project specific integrations. I did plenty of them already. But not the generic one. Project specific is easy and can be done "tomorrow" ;)

@mbj

This comment has been minimized.

Copy link
Owner

mbj commented Jul 7, 2015

@kbrock, @mperham Lets do the following: We simply assume all minitest mutant users will do this exercises TheSubjecOfTest. If they are not willing to annotate: Mutant will not work for them.

That will at least get us started and unblocked.

@mbj

This comment has been minimized.

Copy link
Owner

mbj commented Jul 7, 2015

And instead for having to do a constant reference in this exercises thing, lets allow to pass N mutant expressions.

So you can actually declare a test to match * (any subject under mutation), if you really want this painful way :P

@mbj mbj force-pushed the kbrock:minitest branch 3 times, most recently from 5aa3dad to 876a950 Jul 7, 2015

@mbj

This comment has been minimized.

Copy link
Owner

mbj commented Jul 8, 2015

I pushed my latest version that should pass the specs and supports an extension to minitest specifying the subject of coverage explicitly.

This does the job for the test app mutants tests run against:

class TestAppTest < Minitest::Test
  def self.cover(expression)
    @expression = expression
  end

  def self.cover_expression
    unless @expression
      fail "Cover expression for #{self} is not specified"
    end

    @expression
  end
end

class TestApp::LiteralTest < TestAppTest
  cover 'TestApp::Literal*'

  def test_command
    object = ::TestApp::Literal.new
    subject = object.command('x')

    assert_equal object, subject
  end

  def test_string
    object = ::TestApp::Literal.new
    subject = object.string

    assert_equal 'string', subject
  end
end

I'll try to get a subset of sidekiq passing against this branch on CI later this week.

@mbj mbj force-pushed the kbrock:minitest branch from 876a950 to 6ad341c Jul 8, 2015

@mbj mbj force-pushed the kbrock:minitest branch 2 times, most recently from 3a5a017 to f228e0b Jul 19, 2015

@mbj mbj force-pushed the kbrock:minitest branch 2 times, most recently from 324d3fa to ff925ed Aug 19, 2015

@mbj

This comment has been minimized.

Copy link
Owner

mbj commented Sep 22, 2015

Closing this PR in favor of #445.

@mbj mbj closed this Sep 22, 2015

@kbrock

This comment has been minimized.

Copy link
Contributor

kbrock commented Sep 22, 2015

👍 thanks

@kbrock kbrock deleted the kbrock:minitest branch Sep 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment