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

Unify multi-condition filtering #2874

Merged
merged 1 commit into from
Mar 4, 2021
Merged

Conversation

pirj
Copy link
Member

@pirj pirj commented Feb 20, 2021

Always use all? semantic when applying metadata conditions for filtering.
Previously all? was only used for hooks, and any? was used for everything else (module inclusions, shared example group inclusion, define derived metadata).

Before this change MyModule would be included to example groups defining any of foo: true or bar: true:

RSpec.configure do |c|
  c.include MyModule, :foo => true, :bar => true
end

After the change the same any? behaviour is achievable splitting into two statements:

RSpec.configure do |c|
  c.include MyModule, :foo => true
  c.include MyModule, :bar => true
end

MyModule will only be included in groups with both :foo => true AND :bar => true

RSpec.configure do |c|
  c.include MyModule, :foo => true, :bar => true
end

Fixes #1821

Depends on:

@pirj pirj self-assigned this Feb 20, 2021
@pirj pirj added this to the 4.0 milestone Feb 20, 2021
@pirj pirj force-pushed the unify-multi-condition-filtering-2 branch from 441a29b to 37ec38b Compare February 20, 2021 14:26
@pirj
Copy link
Member Author

pirj commented Feb 20, 2021

The failure is due to https://github.com/alexrothenberg/ammeter/blob/master/lib/ammeter/rspec/generator/example.rb#L14 expecting any? semantic.
I'll send a PR.

pirj added a commit to rspec/rspec-rails that referenced this pull request Feb 20, 2021
See:

- rspec/rspec-core#1821
- rspec/rspec-core#2874
- alexrothenberg/ammeter#64
- example failure (Ammeter::RSpec::Rails::GeneratorExampleGroup that defines destination is not included) https://github.com/rspec/rspec-core/pull/2874/checks?check_run_id=1942170588

Alternative to #2468
pirj added a commit to rspec/rspec-rails that referenced this pull request Feb 20, 2021
See:

- rspec/rspec-core#1821
- rspec/rspec-core#2874
- alexrothenberg/ammeter#64
- example failure (Ammeter::RSpec::Rails::GeneratorExampleGroup that defines destination is not included) https://github.com/rspec/rspec-core/pull/2874/checks?check_run_id=1942170588

Alternative to #2468
pirj added a commit to rspec/rspec-rails that referenced this pull request Feb 20, 2021
See:

- rspec/rspec-core#1821
- rspec/rspec-core#2874
- alexrothenberg/ammeter#64
- example failure (Ammeter::RSpec::Rails::GeneratorExampleGroup that defines destination is not included) https://github.com/rspec/rspec-core/pull/2874/checks?check_run_id=1942170588

Alternative to #2468
spec/rspec/core/configuration_spec.rb Outdated Show resolved Hide resolved
spec/rspec/core/filter_manager_spec.rb Show resolved Hide resolved
@@ -46,7 +46,7 @@ def prune(examples)

examples.select do |ex|
file_scoped_include?(ex.metadata, ids, locations) do
!exclusions.include_example?(ex) && non_scoped_inclusions.include_example?(ex)
(exclusions.empty? || !exclusions.include_example?(ex)) && non_scoped_inclusions.include_example?(ex)
Copy link
Member

Choose a reason for hiding this comment

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

Why? (not had enough ☕ yet 😂 )

Copy link
Member Author

Choose a reason for hiding this comment

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

This was the hardest part of the change.
Without this, exclusions.include_example?(ex) would just always return true, and its negation - false.
This is due to the change in the underlying logic in the case when there are no rules, that is coming from this:

[].all?(&:odd) # new version => true
[].any?(&:odd) # before => false

Without this check, rspec always finishes with 0 examples, 0 failures.

Copy link
Member Author

Choose a reason for hiding this comment

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

InclusionFilter has a similar check:

      def include_example?(example)
        @rules.empty? || super
      end

I thought of refactoring ExclusionRules to a similarly defined exclude_example?, but left it if the later refactoring. Especially since !exclude_example? is hard to wrap your head around quickly when called on an instance of ExclusionRules 😄

Copy link
Member

Choose a reason for hiding this comment

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

I'm having a think about wether include should take this into account, this just seems like a trap currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think there could be several rules in the exclusion filter, and we shouldn't check against them with all??

Copy link
Member

Choose a reason for hiding this comment

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

I feel like this should be encapsulated somewhere, but for now I think this is fine

pirj added a commit to rspec/rspec-rails that referenced this pull request Feb 22, 2021
See:

- rspec/rspec-core#1821
- rspec/rspec-core#2874
- alexrothenberg/ammeter#64
- example failure (Ammeter::RSpec::Rails::GeneratorExampleGroup that defines destination is not included) https://github.com/rspec/rspec-core/pull/2874/checks?check_run_id=1942170588

Alternative to #2468
Previously `all?` was only used for hooks, and `any?` was used for
everything else (module inclusions, shared example group inclusion,
define derived metadata).

**Before** this change `MyModule` would be included to example groups
defining **any** of `foo: true` or `bar: true`:

```ruby
RSpec.configure do |c|
  c.include MyModule, :foo => true, :bar => true
end
```

**After** the change the same `any?` behaviour is achievable splitting
into two statements:

```ruby
RSpec.configure do |c|
  c.include MyModule, :foo => true
  c.include MyModule, :bar => true
end
```

`MyModule` will only be included in groups with **both** `:foo => true`
AND `:bar => true`:

```ruby
RSpec.configure do |c|
  c.include MyModule, :foo => true, :bar => true
end
```

Fixes #1821
@pirj pirj force-pushed the unify-multi-condition-filtering-2 branch from 9c4d9af to d4515ab Compare February 22, 2021 13:39
@pirj
Copy link
Member Author

pirj commented Feb 22, 2021

Green!

@pirj
Copy link
Member Author

pirj commented Mar 4, 2021

This is finally the last one change on the TODO list for 4.0, please take a look. :begger:

What's left is to add deprecation warnings for what we've changed/removed in 4.0 to 3.x.

I've made several attempts of removing WithKeywordsWhenNeeded with no luck removing any. I can keep bashing my head against it, or we can release 4.0 and deal with kwargs after. Up to you to decide, @JonRowe

@pirj pirj merged commit 65cff81 into 4-0-dev Mar 4, 2021
@pirj pirj deleted the unify-multi-condition-filtering-2 branch March 4, 2021 16:52
yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 19, 2021
…i-condition-filtering-2

Unify multi-condition filtering

---
This commit was imported from rspec/rspec-core@65cff81.
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