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

Adds support for the shoulda-context gem #37

Merged
merged 1 commit into from
Apr 16, 2016
Merged

Adds support for the shoulda-context gem #37

merged 1 commit into from
Apr 16, 2016

Conversation

alexpjohnson
Copy link
Contributor

@alexpjohnson alexpjohnson commented Apr 15, 2016

From #32

@ArturT ArturT merged commit 11acf48 into KnapsackPro:master Apr 16, 2016
@@ -48,7 +48,7 @@ def set_test_helper_path(file_path)

def self.test_path(obj)
# Pick the first public method in the class itself, that starts with "test_"
test_method_name = obj.public_methods(false).select{|m| m =~ /^test_/ }.first
Copy link
Member

Choose a reason for hiding this comment

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

When you use the previous version of the code with /^test_/ then test for shoulda-context still passes. It should fail.

@ArturT
Copy link
Member

ArturT commented Apr 16, 2016

@alexpjohnson I had to revert this PR because it doesn't work and test is false positive.

I have here a separate rails app. I added there shoulda-context and I tried to generate report to see if the test file path is detected correctly but it is not.
You can find here more how to test it KnapsackPro/rails-app-with-knapsack#1

Basically what we would like to achieave here is to have correct test file path in the variable full_test_path. Maybe you will find how to do it. I was trying for a while but I couldn't find a way to get test file path based on obj.

      def self.test_path(obj)
        # Pick the first public method in the class itself, that starts with "test_"
        test_method_name = obj.public_methods(false).select{|m| m =~ /^test(:|_)/ }.first
        method_object = obj.method(test_method_name)
        full_test_path = method_object.source_location.first
        parent_of_test_dir_regexp = Regexp.new("^#{@@parent_of_test_dir}")
        test_path = full_test_path.gsub(parent_of_test_dir_regexp, '.')
        # test_path will look like ./test/dir/unit_test.rb
        test_path
      end

@alexpjohnson
Copy link
Contributor Author

Hey @ArturT, I've done a bit more investigating and it seems the root issue is that shoulda-context is breaking the API established by minitest. By dynamically generating methods, they are losing the original source_location of the test. I'm still tinkering around with it, but as that gem is currently designed I don't think it's possible for you to fix it in this gem, nor do I think it's really your responsibility. I've opened up an issue on their gem here. I'll keep tinkering around with it, but hopefully an upstream PR will get merged and then your code should just work :) Thanks for your help, and sorry for the premature PR.

@ArturT
Copy link
Member

ArturT commented Apr 18, 2016

@alexpjohnson Thanks for the update. Hope shoulda-context team will have some tips how to determine the path of the test file. Maybe there is another way than using source_location.

@alexpjohnson
Copy link
Contributor Author

Hey @ArturT I've got a solution that works locally for me. I refactored a bit of the shoulda-context gem to allow passing in the original source location. There's a PR open here.

@ArturT
Copy link
Member

ArturT commented Apr 21, 2016

@alexpjohnson With your fix in shoulda-context there is no need to do changes in knapsack as all methods generated by shoulda-context have prefix test_, right?

@alexpjohnson
Copy link
Contributor Author

Correct. It should all just happen auto-magically :)

@alexpjohnson
Copy link
Contributor Author

Just wanted to keep you up to date in case any of your other users mentioned it.

@doyle doyle deleted the shoulda_context_support branch July 26, 2019 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants