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

Allow to use the matchers with just rspec expectations gem #910

Closed
wants to merge 1 commit into from
Closed

Allow to use the matchers with just rspec expectations gem #910

wants to merge 1 commit into from

Conversation

majioa
Copy link

@majioa majioa commented Mar 2, 2016

Allow to use the matchers for the just rspec expectations, apart the whole

RSpec.

  • It adds the specific test framework that just includes the specified
    libraries directly to RSpec::Matchers.

The addition is covered with an acceptance test.

@@ -94,7 +94,8 @@ def test_helper_files_for(test_framework, libraries)
files = []

if integrates_with_nunit_and_rails?(test_framework, libraries) ||
integrates_with_nunit_only?(test_framework)
integrates_with_nunit_only?(test_framework) ||

Choose a reason for hiding this comment

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

Align the operands of a condition in an if statement spanning multiple lines.

EOT
end

def include(*modules, **options)

Choose a reason for hiding this comment

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

Unused method argument - options. If it's necessary, use _ or _options as an argument name to indicate that it won't be used.

…whole

RSpec.

 * It adds the specific test framework that just includes the specified
   libraries directly to RSpec::Matchers.

The addition is covered with an acceptance test.
@@ -197,6 +197,30 @@ describe Person do
end
```

### RSpec-Expectations

Use this instead of the RSpec ifself in case you just use the specific module gem
Copy link

Choose a reason for hiding this comment

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

I think the English can be improved here, I was a little confused by it. I suggest:

`shoulda-matchers` can be configured to only need the `rspec-expectations` gem instead of the full RSpec suite.

Firstly, include `shoulda-matchers` in your Gemfile:

...it was much harder than I expected to find a good starting point here! Could you perhaps give a one-liner for why you want to use only RSpec-Expectations and not the full suite? Speed? Load-time? Fewer dependencies?

Copy link
Author

Choose a reason for hiding this comment

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

usage with only cucumber )


Shoulda::Matchers.configure do |config|
config.integrate do |with|
with.test_framework :rspec_exp

Choose a reason for hiding this comment

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

I think it would be more expressive / more obvious to use rspec_expectations instead of rspec_exp.

Copy link
Author

Choose a reason for hiding this comment

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

could the exp statement be understood some else than expectations in context of rspec?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Going along with the use case, instead of with.test_framework :rspec_exp, this would be with.test_framework :cucumber.

Copy link
Author

Choose a reason for hiding this comment

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

because the cucumber is the only explicit example, and may be someone will want to use expectations' syntax without cucumber, but on the same way, for example on test-unit replacinf assert syntax.

Copy link
Author

Choose a reason for hiding this comment

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

because the cucumber is the only explicit example, and may be someone will want to use expectations' syntax without cucumber, but on the same way, for example on test-unit replacinf assert syntax.

Copy link
Author

Choose a reason for hiding this comment

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

because the cucumber is the only explicit example, and may be someone will want to use expectations' syntax without cucumber, but on the same way, for example on test-unit replacinf assert syntax.

Copy link
Author

Choose a reason for hiding this comment

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

because the cucumber is the only explicit example, and may be someone will want to use expectations' syntax without cucumber, but on the same way, for example on test-unit replacinf assert syntax.


Then, configure the gem to integrate with RSpec specifying the test framework as `:rspec_exp`:

Shoulda::Matchers.configure do |config|

Choose a reason for hiding this comment

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

Missing a code block here?

Copy link
Author

Choose a reason for hiding this comment

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

here is indent 4 step

end
end

Now you can use matchers in your tests as for RSpec part.

Choose a reason for hiding this comment

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

I think this line is redundant and could be removed for simplicity.

Copy link
Author

Choose a reason for hiding this comment

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

probably


def validate!
return if defined? RSpec::Matchers
raise TestFrameworkNotFound, <<-EOT

Choose a reason for hiding this comment

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

Unless there's a need to explicitly support Ruby < 2.3, this would be nicer with a 'squiggly heredoc': https://infinum.co/the-capsized-eight/multiline-strings-ruby-2-3-0-the-squiggly-heredoc

Copy link
Author

Choose a reason for hiding this comment

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

ok

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with this -- this makes it easier to wrap the message at 80 characters.

Copy link
Collaborator

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Hey @majioa. Sorry that we haven't responded to this yet. 😬 The bad news is that we're in a code freeze right now, so we can't merge this in quite yet, but the good news is that we almost have 4.0.0 out, so we will start revisiting some of these older PRs once we have that completed.

That said, I took some time to check over your PR, and I had a thought. I wonder if it would make more sense to reframe this as integrating less with rspec-expectations and more with Cucumber. I can't imagine anyone else would want to use shoulda-matchers just with rspec-expectations, unless they were using it with Cucumber. Certainly there are not many people who would want to even use shoulda-matchers apart from Minitest or RSpec, but in making this integration about Cucumber, I feel like it would fall logically alongside what is already there and would be more understandable to people who are scrolling through the README for the first time. Plus -- and perhaps more importantly -- I feel like it would help us as maintainers understand why this code is here and what purpose it serves. What do you think about this? I added some comments below to point out potential changes.

I had another thought while writing the above. I have been under the understanding that you use Cucumber to write unit tests. However, the integration test you wrote has me wondering if perhaps that's not what you're doing -- in which case, everything I wrote in this PR wouldn't apply. Can you enlighten me?

@@ -197,6 +197,30 @@ describe Person do
end
```

### RSpec-Expectations
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be renamed to "Cucumber", and this whole section would talk about that instead of mentioning rspec-expectations.


Shoulda::Matchers.configure do |config|
config.integrate do |with|
with.test_framework :rspec_exp
Copy link
Collaborator

Choose a reason for hiding this comment

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

Going along with the use case, instead of with.test_framework :rspec_exp, this would be with.test_framework :cucumber.

module Integrations
module TestFrameworks
# @private
class RspecExpectations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of RspecExpectations, this would be Cucumber, and everything in this file would talk about that rather than rspec-expectations.


def validate!
return if defined? RSpec::Matchers
raise TestFrameworkNotFound, <<-EOT
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with this -- this makes it easier to wrap the message at 80 characters.

end

def include(*modules, **_options)
RSpec::Matchers.send(:include, *modules)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So now, instead of including modules into RSpec::Matchers (which requires understanding that RSpec::Matchers is itself mixed into Object), this could be World(*modules) (which is a public API).

@@ -0,0 +1,59 @@
require 'acceptance_spec_helper'

describe 'shoulda-matchers integrates libs for rspec-expectations framework' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file would be renamed/reframed to represent Cucumber and would set up a Cucumber project.

@@ -118,6 +119,10 @@ def integrates_with_rspec?(test_framework)
test_framework == :rspec
end

def integrates_with_rspec_expectations?(test_framework)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be integrates_with_cucumber?. You'd have to modify test_helper_files_for to do:

if integrates_with_cucumber?(test_framework)
  files << 'features/support/env.rb'
end

end
FILE

append_rake_task 'rspec_exp', 'environment', <<-CODE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of appending to a rake task like this, I would actually write this to a file. In the case of Cucumber this could be features/cucumber_integration.feature or whatever you want to call it. Unless that's not how you actually write your tests? I'm a bit confused here. Please enlighten me :)

Copy link
Author

Choose a reason for hiding this comment

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

@mcmire If we suppose that the case is limited by cucumber case that should be true, but because the cucumber is the only explicit example, and may be someone will want to use expectations' syntax without cucumber, but on the same way, for example on test-unit replacing assert syntax. That is why I just used the common case called rspec-exp/rspec-expectations to describe the feature. Well it may be not so correct naming convention for the case, but cucumber is also not a proper naming.

@majioa
Copy link
Author

majioa commented Oct 22, 2018

@mcmire If we suppose that the case is limited by cucumber case that should be true, but because the cucumber is the only explicit example, and may be someone will want to use expectations' syntax without cucumber, but on the same way, for example on test-unit replacing assert syntax. That is why I just used the common case called rspec-exp/rspec-expectations to describe the feature. Well it may be not so correct naming convention for the case, but cucumber is also not a proper naming. May be it should have name expectation-commons?

@mcmire
Copy link
Collaborator

mcmire commented Oct 22, 2018

@majioa My sense is that making a more generic adapter would be premature abstraction. If someone else comes forward later and says that they want to use RSpec syntax with Test::Unit down the line, then I would take that into consideration, but you're literally the only person who's proposed anything like this, and I don't want to confuse the 99% of the userbase who is perfectly happy using shoulda-matchers with the full RSpec experience.

That said, can you provide an example of the type of test that you write? I'm not still clear on how you plan on using this.

@mcmire
Copy link
Collaborator

mcmire commented May 6, 2020

Hey folks. In an effort to lighten our load as maintainers and be able to serve you better in the future, the shoulda-matchers team is working on cleaning out the cobwebs in this repo by pruning the backlog. As there are few of us, there are a lot of items that will simply never earn our attention in a reasonable time frame, and rather than giving you an empty promise, we think it makes more sense to focus on more recent issues. That means, unfortunately, that we must close this PR.

Don't take this the wrong way: our aim is not to diminish the effort people have made or dismiss problems that have been raised. If you feel that we should reopen this PR, then please let us know so that we can reprioritize it. Thanks!

@mcmire mcmire closed this May 6, 2020
@majioa
Copy link
Author

majioa commented May 6, 2020

@mcmire
Copy link
Collaborator

mcmire commented May 6, 2020

Oh very nice! I've added your extension to the README: https://github.com/thoughtbot/shoulda-matchers#extensions

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

Successfully merging this pull request may close these issues.

None yet

4 participants