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

Add test minitest integration #445

Merged
merged 2 commits into from Nov 19, 2018
Merged

Add test minitest integration #445

merged 2 commits into from Nov 19, 2018

Conversation

@mbj
Copy link
Owner

@mbj mbj commented Sep 22, 2015

[fix #92]

References: #330.

@kbrock
Copy link
Contributor

@kbrock kbrock commented Sep 22, 2015

could you add a check list or some ideas what may need to be added here?

@mbj
Copy link
Owner Author

@mbj mbj commented Sep 22, 2015

could you add a check list or some ideas what may need to be added here?

Will do. Right now it needs someone who tests it on a non trivial project, that has real unit tests. That will likely be myself. And I'm blocked by time right now.

@mperham
Copy link

@mperham mperham commented Sep 22, 2015

I'll play with it today on sidekiq's suite.

@mbj
Copy link
Owner Author

@mbj mbj commented Sep 22, 2015

I'll play with it today on sidekiq's suite.

I tried with sidekiq for maybe 10min back one month or so. But could not isolate a test / subject set that is a "real unit" test for testing the rspec integration.

By that I mean that most of the tests (and maybe even the global setup, I do not recall precicely) require redis / and or cause side effects.

You should start with -j1 to limit concurrency. I had some local commits that made the sidekiq test suite more accessible for mutant, but I lost them in the chaos that is my current live (sick for 4+ weeks).

Conceptually I think that sidekiq might not be the best project to verify the mutant-minitest integration. Since its domain is mostly managing nondeterministic things (threads, network, ...).

Not that I think sidekiq and mutant "cannot" play well together, but I think its at too big step. It would be easier to start with a smaller lib that does not have IO etc to validate the integration is correct, before using the integration on something bigger and running into false positives that burn time.

@dogweather
Copy link

@dogweather dogweather commented Oct 4, 2015

How would I try this out?

@mbj
Copy link
Owner Author

@mbj mbj commented Nov 3, 2015

How would I try this out?

No time at all sorry. Someone with a less "global state hitting" minitest project should try.

@dogweather
Copy link

@dogweather dogweather commented Nov 3, 2015

I mean just, how would I invoke this from the command line? minitest in place of rspec?

@mbj
Copy link
Owner Author

@mbj mbj commented Nov 3, 2015

I mean just, how would I invoke this from the command line? minitest in place of rspec?

--use minitest yes. And use this branch.

@dogweather
Copy link

@dogweather dogweather commented Nov 3, 2015

I gave it a try on a Rails project, and it executed part-way but then had an error:

RAILS_ENV=test bundle exec mutant -r ./config/environment -I test --use minitest Agency

Output:

Mutant configuration:
Matcher:         #<Mutant::Matcher::Config match_expressions: [Agency]>
Integration:     Mutant::Integration::Minitest
Expect Coverage: 100.00%
Jobs:            8
Includes:        ["test"]
Requires:        ["./config/environment"]
Subjects:        14
Mutations:       729
Kills:           0
Alive:           0
Runtime:         0.00s
Killtime:        0.00s
Overhead:        Inf%
Coverage:        100.00%
Expected:        100.00%
Active subjects: 0
/Users/robb/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/bundler/gems/mutant-36269e744bcd/lib/mutant/integration/minitest.rb:60:in `expression_syntax': undefined method `cover_expression' for BusinessesControllerTest:Class (NoMethodError)
    from /Users/robb/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/bundler/gems/mutant-36269e744bcd/lib/mutant/integration/minitest.rb:138:in `construct_test'
    from /Users/robb/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/bundler/gems/mutant-36269e744bcd/lib/mutant/integration/minitest.rb:123:in `block in all_tests_index'
    from /Users/robb/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/bundler/gems/mutant-36269e744bcd/lib/mutant/integration/minitest.rb:122:in `each'
    from /Users/robb/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/bundler/gems/mutant-36269e744bcd/lib/mutant/integration/minitest.rb:122:in `each_with_object'
    from /Users/robb/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/bundler/gems/mutant-36269e744bcd/lib/mutant/integration/minitest.rb:122:in `all_tests_index'
    from /Users/robb/.rbenv/versions/2.2.3/lib/ruby/gems/2.2.0/gems/memoizable-0.4.2/lib/memoizable/method_builder.rb:117:in `call'

(etc.)

@mbj
Copy link
Owner Author

@mbj mbj commented Nov 3, 2015

Likely this is triggered from missing autoload triggers. Typical rails symptom. Try to explicitly require the file that defines that method. Or ideally test on a non rails project.

@dogweather
Copy link

@dogweather dogweather commented Nov 3, 2015

It seems that Mutant expects the missing method #cover_expression. (?) I can't find any references to that online. What should be providing that?

        # Cover expression syntaxes
        #
        # @return [Array<String>]
        #
        # @api private
        def expression_syntax
          klass.cover_expression
        end

https://github.com/mbj/mutant/blob/feature/minitest-integration/lib/mutant/integration/minitest.rb#L54-L61

@mbj
Copy link
Owner Author

@mbj mbj commented Nov 3, 2015

@dogweather Ahh, I remember see the discussion in #330.

@mbj mbj added the enhancement label Nov 16, 2015
@mbj
Copy link
Owner Author

@mbj mbj commented Dec 20, 2015

Depends on #508 from software rusting.

@mbj mbj force-pushed the feature/minitest-integration branch 3 times, most recently from ebe513a to 5262a1e Dec 20, 2015
@mbj mbj removed the in-progress label Dec 20, 2015
@mbj
Copy link
Owner Author

@mbj mbj commented Dec 20, 2015

@dkubb Ready for review. Includes #508 so should not be merged before.

@mbj mbj force-pushed the feature/minitest-integration branch from e431695 to 8315a4d Dec 20, 2015
output = StringIO.new
reporter = ::Minitest::SummaryReporter.new(output)

start = Time.now

This comment has been minimized.

@dkubb

dkubb Dec 21, 2015
Collaborator

I would align the assignment operators here.

This comment has been minimized.

@backus

backus Dec 21, 2015
Contributor

Instead of recording Time.now and doing Time.now - start later could you use Benchmark.realtime?

This comment has been minimized.

@dkubb

dkubb Dec 28, 2015
Collaborator

@backus I agree, that is a much better idea.

Another nice side benefit is that you eliminate some extra noise from StringIO#rewind and StringIO#read since you could wrap the passed = test_cases.all? { |test| test.call(reporter) } line only.

# @return [Array<TestCase>]
def test_case(runnable)
runnable.runnable_methods.each_with_object([]) do |method_name, test_cases|
next unless method_name.start_with?(TEST_METHOD_PREFIX)

This comment has been minimized.

@dkubb

dkubb Dec 21, 2015
Collaborator

I wonder if you couldn't use Enumerable#select to match runnable methods with a given prefix, allowing you to just use map instead of Enumerable#each_with_object ?

This comment has been minimized.

@backus

backus Dec 22, 2015
Contributor

Adding onto @dkubb's comment this seems to be the only TEST_METHOD_PREFIX is used so that could be changed to TEST_METHOD_PATTERN = /\Atest_/.freeze and instead of Enumerable#select you could just do a grep(TEST_METHOD_PATTERN).map

This comment has been minimized.

@dkubb

dkubb Dec 28, 2015
Collaborator

Also you wouldn't even need to use map since grep with a block acts the same as map. This method could be rewritten as:

TEST_METHOD_PREFIX = /\Atest_/.freeze

# ...

def test_case(runnable)
  runnable.runnable_methods.grep(TEST_METHOD_PREFIX) do |method_name|
    TestCase.new(runnable, method_name)
  end
end

Or if you wanted to get fancy with Method#curry:

TEST_METHOD_PREFIX = /\Atest_/.freeze

# ...

def test_case(runnable)
  test_case = TestCase.method(:new).curry(2)[runnable]
  runnable.runnable_methods.grep(TEST_METHOD_PREFIX, &test_case)
end

This comment has been minimized.

This comment has been minimized.

@dkubb

dkubb Dec 28, 2015
Collaborator

@backus oh, nice find! I'd guess we can just get by with using Enumerable#map here instead.

gem.name = 'mutant-minitest'
gem.version = Mutant::VERSION.dup
gem.authors = ['Markus Schirp']
gem.email = ['mbj@schirp-dso.com']

This comment has been minimized.

@dkubb

dkubb Dec 21, 2015
Collaborator

You could use %w[...] here.

gem.extra_rdoc_files = %w[TODO LICENSE]

gem.add_runtime_dependency('mutant', "~> #{gem.version}")
gem.add_runtime_dependency('minitest', '>= 5.3.0')

This comment has been minimized.

@dkubb

dkubb Dec 21, 2015
Collaborator

Can you use ~> here, or ~> and >=? I generally would not recommend using >= by itself; especially when you don't control the upstream gem.

This comment has been minimized.

@mbj

mbj Dec 21, 2015
Author Owner

Good catch.

gem.add_runtime_dependency('mutant', "~> #{gem.version}")
gem.add_runtime_dependency('minitest', '>= 5.3.0')

gem.add_development_dependency('bundler', '~> 1.3', '>= 1.3.5')

This comment has been minimized.

@dkubb

dkubb Dec 21, 2015
Collaborator

I believe bundler's version has changed a bit since this was written.

This comment has been minimized.

@mbj

mbj Dec 21, 2015
Author Owner

I think the bundler version should not be specified. I'm not aware about any bug this would prevent. I'll drop it here.

require 'minitest/autorun'

# require spec support files and shared behavior
Dir[File.expand_path('../{support,shared}/**/*.rb', __FILE__)].each do |file|

This comment has been minimized.

@dkubb

dkubb Dec 21, 2015
Collaborator

I would sort the files to make sure they are always required in the same order.

def self.autorun
end

end # Minitest

This comment has been minimized.

@backus

backus Dec 21, 2015
Contributor

Could rewrite this monkey patch on one line with something like

Minitest.define_singleton_method(:autorun) { }

This comment has been minimized.

@mbj

mbj Dec 21, 2015
Author Owner

Interesting idea to shorten it like this. I wounder how we can apply the axioms to deterministically prefer one form over the other. //cc @dkubb.

This comment has been minimized.

@dkubb

dkubb Dec 28, 2015
Collaborator

I'm kind of partial to the def form myself. I generally prefer to use built-in language syntax over meta-programming methods, assuming the end result is equal.

This comment has been minimized.

@mbj

mbj Dec 29, 2015
Author Owner

built-in language syntax over meta-programming methods, assuming the end result is equal.

Yes. And the rule of least power supports this as the metaprogramming closure has a much wider scope. Using the primary syntax has "less power" (scope wise) so should be preferred here.

This comment has been minimized.

@backus

backus Dec 29, 2015
Contributor

If you want to stick with the primary syntax here you could still cut two lines by instead writing

def Minitest.autorun
end

I know this syntax is usually frowned upon but it might make sense in a context like this. It also has the benefit then of not pinning YARD documentation to the toplevel Minitest namespace.

This comment has been minimized.

@mbj

mbj Jan 2, 2016
Author Owner

Reopening the scope Minitest is not needed technically, but maybe for consistency?

# Minitest integration
class Minitest < self
TEST_METHOD_PREFIX = 'test_'.freeze
TEST_FILE_PATTERN = './test/**/{test_*,*_test}.rb'.freeze

This comment has been minimized.

@backus

backus Dec 21, 2015
Contributor

I pulled down the 100 most popular gems depending on minitest to see if the convention was this clear cut. A few differences I noticed:

  • The json gem which is now part of ruby's standard lib uses a "tests" directory which does not match this glob
  • The excon gem uses a "tests" directory then I think their test files have a _tests.rb suffix

Still looking for other deviations right now so I'll probably update this comment.

My conclusion here isn't necessarily that the glob should change though. Instead, I think this indicates that it might be important for to have a --test-pattern flag in the future which makes this configurable.

This comment has been minimized.

@mbj

mbj Dec 21, 2015
Author Owner

Thx for your investigation.

Yeah. These integration specific flags need to happen. Where I think that we even more discovered need for a persistent config file.

This comment has been minimized.

@backus

backus Dec 21, 2015
Contributor

No problem. I stopped looking for deviations since it sounds like you agree this needs to be configurable. As one last check I grepped out the gem.test_files setting in each gemspec. Here are the unique entries (minus a few weirdos):

Dir.glob("spec/**/*")
Dir["spec/**/*.rb"]
Dir["test/spec_*.rb"]
Dir["test/test_*.rb"]
`git ls-files -- Appraisals {spec}/*`.split("\n")
`git ls-files -- spec/*`.split("\n")
`git ls-files -- test/*`.split("\n")
`git ls-files -- test`.split($/)
`git ls-files -- {spec}/*`.split("\n")
`git ls-files -- {test,spec,features}/*`.split("\n")
`git ls-files -- {test}/*`.split("\n")
`git ls-files spec/{unit,integration}`.split($/)
gem.files.grep(%r{^(test|spec|features)/.*\.rb})
gem.files.grep(%r{^(test|spec|features)/})
gem.files.grep(%r{^(test|spec|features)/}).grep(%r{/test_[^/]+\.rb$})
gem.files.grep(%r{^spec/})
s.files.select { |path| path =~ /^[spec|tests]\/.*_[spec|tests]\.rb/ }
s.files.select {|path| path =~ /^test\/.*_test.rb/}
spec.files.grep(%r{^(test|spec|features)/})
spec.files.grep(%r{^(test|spec|features|acceptance)/})
spec.files.grep(%r{^(test|spec|features|examples)/})
spec.files.grep(%r{^test/})
@mbj
Copy link
Owner Author

@mbj mbj commented Sep 5, 2018

hi @mbj! I know you haven't looked at this in a while, but would you mind if I took over this work?

I'd be super happy if you did so.

Feel free to open a fresh PR. I'd close this one and reference it.

@mbj mbj force-pushed the feature/minitest-integration branch from 6bc8af0 to cc02977 Nov 12, 2018
@mbj
Copy link
Owner Author

@mbj mbj commented Nov 12, 2018

So the news is I've got a sponsor for moving this feature into a released version:

  • Initially we'll have to keep the cover_expression "hack".
  • We'll use auom as the corpus test, which will get a separate minitest suite. This is also nice to compare mutant side by side on both rspec and minitest (I think minitest will be faster).
@mbj mbj force-pushed the feature/minitest-integration branch 6 times, most recently from 03e8107 to 5917ad2 Nov 12, 2018
@mbj
Copy link
Owner Author

@mbj mbj commented Nov 16, 2018

Prelimiary results are good, AUOM is fully covered. I do want to polish the cover expression mechanism a bit more, and possibly add a bit better "selection reporting" before release.

@mbj
Copy link
Owner Author

@mbj mbj commented Nov 16, 2018

This is on how an mutant minitest integration may look like: mbj/auom#19

@mbj mbj force-pushed the feature/minitest-integration branch 3 times, most recently from 7b6a32f to 3b1e78a Nov 17, 2018
[fix #92]
@mbj mbj force-pushed the feature/minitest-integration branch 9 times, most recently from 0ac8999 to 1d1e544 Nov 18, 2018
@mbj mbj force-pushed the feature/minitest-integration branch from 1d1e544 to 9ceb2bd Nov 19, 2018
@mbj mbj merged commit 5690106 into master Nov 19, 2018
5 checks passed
5 checks passed
ci/circleci: integration_mutation_generation Your tests passed on CircleCI!
Details
ci/circleci: integration_rspec Your tests passed on CircleCI!
Details
ci/circleci: metrics Your tests passed on CircleCI!
Details
ci/circleci: mutant Your tests passed on CircleCI!
Details
ci/circleci: unit_specs Your tests passed on CircleCI!
Details
@mbj mbj deleted the feature/minitest-integration branch Nov 19, 2018
@mbj
Copy link
Owner Author

@mbj mbj commented Nov 20, 2018

There is a pre-release on rubygems. v0.0.2: https://rubygems.org/gems/mutant-minitest

The "official" release is pending, will be paired with better docs. Version will than jump to the mutant version, which is 0.8.19 as time of writing this.

I'd apprechiate the following:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

You can’t perform that action at this time.