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

Change the expectation for predicate methods #2353

Merged
merged 2 commits into from Jun 10, 2020
Merged

Conversation

pirj
Copy link
Member

@pirj pirj commented Jun 7, 2020

Predicate methods typically return true or false, and should only exist for the uncountable and discrete, e.g. use_transactional_fixtures?, but not max_formatted_output_length?.

This relates to the changes made in rspec/rspec-core#2736

@pirj pirj self-assigned this Jun 7, 2020
@pirj
Copy link
Member Author

pirj commented Jun 7, 2020

I guess those two PRs will be red until any one of those PRs is merged. Unfortunately, our build system is not sophisticated enough to check out the "same branch" to test multi-repo changes.

@pirj
Copy link
Member Author

pirj commented Jun 7, 2020

  1. We seem to miss include_examples "adds setting" for use_active_record and file_fixture_path
  2. Weird, what fixture_path?/file_fixture_path? are for? Maybe drop them?

Should I fix it here?

@benoittgt
Copy link
Member

Thanks @pirj

For 1, I think it could be addressed in another PR
For 2, where do you see fixture_path?/file_fixture_path??

@pirj
Copy link
Member Author

pirj commented Jun 8, 2020

For 2, where do you see fixture_path?/file_fixture_path??

I don't see it used anywhere as well, but if you add this here:

expect(group).to respond_to(:fixture_path?)

it passes, same with file_fixture_path?.

@JonRowe
Copy link
Member

JonRowe commented Jun 8, 2020

rspec-rails overrides rspec-core configuration, so this could be made to work, the question is should it.

lib/rspec/rails/configuration.rb Outdated Show resolved Hide resolved
lib/rspec/rails/configuration.rb Outdated Show resolved Hide resolved
lib/rspec/rails/configuration.rb Outdated Show resolved Hide resolved
Predicate methods typically return true or false, this is part of an
associated bug-fix in rspec/rspec-core#2736 but as rspec-rails is now
a separate entity we must account for this ourselves.

Note some predicates are here by default settings and may be able to
be tidied up in `rspec-rails` 5.
@JonRowe
Copy link
Member

JonRowe commented Jun 10, 2020

@pirj I've taken this as base and improved it. Assuming it goes green we should merge it.

My reasoning here:

  • The predicates already exist so can't be removed until rspec-rails 5
  • We shouldn't depend on a specific version of rspec-core, so just fix it ourselves.

@pirj
Copy link
Member Author

pirj commented Jun 10, 2020

Looks good! 👍

However, regarding backwards-compatibility. We're changing what those predicates return, don't we? Previously file_fixture_path? was returning a string, but now it's a true/false.

@JonRowe
Copy link
Member

JonRowe commented Jun 10, 2020

However, regarding backwards-compatibility. We're changing what those predicates return, don't we? Previously file_fixture_path? was returning a string, but now it's a true/false.

Yes which is my concern with rspec/rspec-core#2736 too but I've come to the decision this can be safely considered a bug fix, its not expected that predicate methods would be used except as truthy/falsely

@JonRowe
Copy link
Member

JonRowe commented Jun 10, 2020

This is green on CI, just hasn't updated Github.

@JonRowe JonRowe merged commit e3a0353 into master Jun 10, 2020
@JonRowe JonRowe deleted the be_true_to_yourself branch June 10, 2020 10:32
@@ -105,7 +106,35 @@ def render_views
end

def render_views?
rendering_views
rendering_views?
end
Copy link
Member Author

Choose a reason for hiding this comment

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

previous definition of use_transactional_fixtures? was here in rspec-core builds. We'll have to resort to attr_accessor.

This was referenced Mar 15, 2021
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.

None yet

3 participants