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

Fix minitest test_prelude #102

Closed

Conversation

flavorjones
Copy link

This PR addresses two issues:

  1. In lib/hoe/test.rb, the use of self.test_prelude within a Minitest::TestTask.create block doesn't do what's intended, because the block is being instance_evaled in the context of the TestTask. As a result, t.test_prelude is always blank, breaking test_prelude functionality completely.

This PR uses a temporary variable outside the block to pass this value into the block:

+        test_prelude = self.test_prelude
         Minitest::TestTask.create :test do |t|
-          t.test_prelude = self.test_prelude
+          t.test_prelude = test_prelude
           t.libs += Hoe.include_dirs.uniq
         end
  1. In lib/minitest/test_task.rb the order of the framework snippet and the prelude snippet are reversed as compared to the pre-v3.18.0 implementation.

Where previously this code put the prelude in front of the framework snippet:

    tests = ["rubygems"]
    tests << framework if framework
    tests << test_globs.sort.map { |g| Dir.glob(g) }
    tests.flatten!
    tests.map! { |f| %(require "#{f}") }

    tests.insert 1, test_prelude if test_prelude

The current code reverses this sense:

       runner = []
       runner << framework
       runner << test_prelude if test_prelude
       runner.concat tests
       runner = runner.join "; "

This PR restores the original order, which as it turns out is critical to getting SimpleCov to work with Minitest. (See sparklemotion/nokogiri#1965 for the SimpleCov-related problem that spawned this particular yak shave!)

to prepare for mutating the object under test in the next commit
to ensure the test command matches pre-v3.18.0 behavior
flavorjones added a commit to sparklemotion/nokogiri that referenced this pull request Jan 27, 2020
unless and until seattlerb/hoe#102 is merged
to fix our coverage calculation issue

related to #1965
flavorjones added a commit to sparklemotion/nokogiri that referenced this pull request Feb 2, 2020
to work around SimpleCov coverage being broken until
seattlerb/hoe#102 is merged
@flavorjones
Copy link
Author

@zenspider just a gentle ping on this - let me know if you'd like a different approach taken to address my issues. thanks for all your work.

@zenspider
Copy link
Member

Just saw your ping... I'm poking. I think your approach is fine, but your tests all pass w/o any patch to lib so they won't prevent a regression.

@zenspider
Copy link
Member

AH. OK... I figure out what's going on. The tests are always going against the old make_test_cmd even when sometimes it is set up to go against the new TestTask which has its own (broken) make_test_cmd.... lemme try to modify the tests to expose.

@zenspider
Copy link
Member

Please verify:

  # here just for comparison:
  EXPECTED = %W[-w -Ilib:bin:test:.
                -e 'require "rubygems"; %srequire "test/test_hoe_test.rb"'
                --].join(" ") + " "

  MT_EXPECTED = %W[-Ilib:test:. -w
                   -e '%srequire "test/test_hoe_test.rb"'
                   --].join(" ") + " "

  require "minitest/test_task" # currently in hoe, but will move

  def test_make_test_cmd_for_minitest
    skip "Using TESTOPTS... skipping" if ENV["TESTOPTS"]

    framework = %(require "minitest/autorun"; )

    @tester = Minitest::TestTask.create :test do |t|
      t.libs += Hoe.include_dirs.uniq
      t.test_globs = ["test/test_hoe_test.rb"]
    end

    assert_equal MT_EXPECTED % [framework].join("; "), @tester.make_test_cmd
  end

  def test_make_test_cmd_for_minitest_prelude
    skip "Using TESTOPTS... skipping" if ENV["TESTOPTS"]

    prelude = %(require "other/file")
    framework = %(require "minitest/autorun"; )

    @tester = Minitest::TestTask.create :test do |t|
      t.test_prelude = prelude
      t.libs += Hoe.include_dirs.uniq
      t.test_globs = ["test/test_hoe_test.rb"]
    end

    assert_equal MT_EXPECTED % [prelude, framework].join("; "), @tester.make_test_cmd
  end

@flavorjones
Copy link
Author

The tests you put inline above look good to me, and reflect the expected behavior. Thanks!

@flavorjones
Copy link
Author

Would you like me to resubmit this PR with your suggested test structure?

@zenspider
Copy link
Member

I already got ya (9b07634).

I should have closed this. I'll ship nowish.

@zenspider zenspider closed this Feb 9, 2020
@flavorjones
Copy link
Author

Thank you!

@flavorjones flavorjones deleted the fix-minitest-prelude branch February 10, 2020 11:29
@seattlerb seattlerb locked and limited conversation to collaborators Jan 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants