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

Avoid be_falsey and be_truthy #2736

Merged
merged 9 commits into from Jun 18, 2020
Merged

Conversation

marcandre
Copy link
Contributor

@marcandre marcandre commented Jun 4, 2020

This PR first fixes Configuration#any_predicate? to return true/false instead of any_predicate.

No spec fail for this change, because all specs on those predicates use be_truthy, like this one which passed even though config.custom_option? used to be a simple alias for config.custom_option and return 'a value'.

Second commit removes all uses of be_truthy and be_falsey except the two for testing the matchers themselves. I believe most of these date from before they were renamed to avoid confusion

Most of these changes are trivial be_truthy => be(true). A non-trivial example is this spec that was not updated now that fail-fast defaults to 1 instead of true.

Notes:
* no idea why the `remove_method` is needed now and wasn't needed previously
* no spec failure as specs use `be_falsey` and `be_truthy`
pirj added 3 commits June 5, 2020 23:55
Warning text that includes file paths was cut off when a spec was
failing due to a Ruby warning.
We define the reader as a method down the lines anyway.
core_spec was issuing a warning that this method is redefined.
`define_aliases` actually only defined one.

`define_predicate_for` was always passed a single argument.
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

I dared to push a small fix for profile_examples.

lib/rspec/core/configuration.rb Outdated Show resolved Hide resolved
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

I'm convinced.

Also, wonderful branch name.

lib/rspec/core/world.rb Show resolved Hide resolved
spec/rspec/core/example_group_spec.rb Show resolved Hide resolved
spec/rspec/core/rake_task_spec.rb Show resolved Hide resolved
Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

It seems these changes cause enough of a shift in our public api to break rspec-rails specs?

I've left some feedback that needs addressing as well...

features/clear_examples.feature Outdated Show resolved Hide resolved
features/clear_examples.feature Outdated Show resolved Hide resolved
lib/rspec/core/configuration.rb Outdated Show resolved Hide resolved
@@ -14,18 +14,14 @@ Feature: custom settings
expect(RSpec.configuration.custom_setting).to be_nil
end

it "acts false by default" do
expect(RSpec.configuration.custom_setting).to be_falsey
end
Copy link
Member

Choose a reason for hiding this comment

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

This is part of the documentation, not a copy of another spec so should be restored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it thinking that:

  1. be_falsey should not be encouraged
  2. it is completely redundant with the previous spec (impossible to pass this and not pass the previous spec)
  3. it does not document in any way custom_setting, only that we can always expect(nil).to be_falsey

@@ -296,16 +296,16 @@
end

describe "--fail-fast" do
it "defaults to false" do
expect(parse_options[:fail_fast]).to be_falsey
it "defaults to nil" do
Copy link
Member

Choose a reason for hiding this comment

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

These concern me as they represent a change in how we document them, if they are wrong I almost want to fix them rather than the specs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't check the implementation, but I find it perfect that the setting is nil if there's no CLI option, and 1 or false if there's an explicit CLI option. For example it makes it possible to override a nil setting with a local setting file, but false should not be overriden.
Or are you talking about 1 vs true?

@marcandre
Copy link
Contributor Author

rspec-rails specs

Yes, the rspec-rails have a few tests for "Configuration adds settings '...'" which now have errors because the predicates now return false/true instead of nil/whatever_value.

IMO these tests should consider that add_setting is not their responsibility (it is rspec-core's) and could simply check that they add_setting was called, or a more basic check that readers/writers exist, not exactly how they behave.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

RSpec Rails failures seem to be mostly caused by the change of what predicates return now !!value.
They shouldn't expect a nil where it's actually a false.
I'll take a stab at fixing tomorrow.

lib/rspec/core/configuration.rb Show resolved Hide resolved
pirj added a commit to rspec/rspec-rails that referenced this pull request 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 changes made in rspec/rspec-core#2736
@pirj
Copy link
Member

pirj commented Jun 7, 2020

Fixed the spec in RSpec Rails, but it fails there because it depends on master rspec-core that doesn't have this change merged and still returns nil instead of false for global_fixtures?, use_transactional_fixtures?, use_instantiated_fixtures?, rendering_views?, and fixture_path?.

@marcandre
Copy link
Contributor Author

Fixed the spec in RSpec Rails, but it fails there because it depends on master rspec-core that doesn't have this change merged and still returns nil instead of false for global_fixtures?, use_transactional_fixtures?, use_instantiated_fixtures?, rendering_views?, and fixture_path?.

😆 if you want, you could commit an intermediate step with is_falsey and is_truthy that would pass on all versions. I still have a feeling these tests are testing the correctness of an external library.

@pirj
Copy link
Member

pirj commented Jun 7, 2020

these tests are testing the correctness of an external library

We prefer to be on the safe side.

@pirj pirj requested review from JonRowe and benoittgt June 9, 2020 19:54
@pirj
Copy link
Member

pirj commented Jun 9, 2020

I propose to merge this despite underlying rspec-rails failure. That failure is fixed in rspec/rspec-rails#2353

That RSpec Rails PR is red since its specs expect predicate methods to return a boolean. Before this change, they were returning the same value as the reader (which doesn't make much sense to me).

JonRowe pushed a commit to rspec/rspec-rails that referenced this pull request Jun 10, 2020
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 JonRowe merged commit d10e82d into rspec:master Jun 18, 2020
JonRowe added a commit that referenced this pull request Jun 18, 2020
JonRowe added a commit that referenced this pull request Jun 18, 2020
JonRowe added a commit that referenced this pull request Jun 18, 2020
pirj added a commit to rspec/rspec-rails that referenced this pull request Aug 2, 2020
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.
MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020
MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021
…e_to_yourself

Avoid be_falsey and be_truthy 

---
This commit was imported from rspec/rspec-core@97a6f23.
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 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