Deprecate top_level_group? and test it in a Cop class #1976

corsonknowles · 2024-10-14T22:47:39Z

In this PR, we add a deprecation message for top_level_group?, which has no callers in current Rubocop/RSpec.

top_level_group? was confirmed unused by @bquorning here:

We test this method using some stubbing in a Cop class that includes the mixin.

The specs can be removed along with the method in the next Major version update.

Split from:

Complete line coverage for Metadata mixin #1970

This PR serves the goal of completing line coverage for this repo ~~and unblocks~~ helps unblock:

Update .simplecov #1971

Before submitting the PR make sure the following are checked:

Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Updated documentation.
Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

Added the new cop to config/default.yml.
The cop is configured as Enabled: pending in config/default.yml.
The cop is configured as Enabled: true in .rubocop.yml.
The cop documents examples of good and bad code.
The tests assert both that bad code is reported and that good code is not reported.
Set VersionAdded: "<<next>>" in default/config.yml.

If you have modified an existing cop's configuration options:

Set VersionChanged: "<<next>>" in config/default.yml.

pirj · 2024-10-16T05:22:40Z

lib/rubocop/cop/rspec/mixin/top_level_group.rb

@@ -29,6 +34,7 @@ def on_top_level_example_group(_node); end
        def on_top_level_group(_node); end

        def top_level_group?(node)


Suggested change

def top_level_group?(node)

# @private

# @deprecated <some description on how to replace>

def top_level_group?(node)

This is more a reminder for us not to forget to remove in 4.0
Should we file a ticket?

Like an Issue? Or is there an internal ticketing system?

I added the @deprecated declaration, but I'm still not sure what filing a ticket entails.

Yeah, we just use issues. I created a 4.0 milestone and opened #1986.

CHANGELOG.md

pirj · 2024-10-16T05:28:50Z

lib/rubocop/cop/rspec/mixin/top_level_group.rb

+        DEPRECATED_PRIVATE_METHOD_WARNING =
+          'top_level_group? is deprecated and will be ' \
+          'removed in the next major version of Rubocop/RSpec.' \
+          'Please copy the method if you need it for your mixin.'


Is it a good advice to copy?
We’ve stopped using it for a reason?
Was that performance? Should we say to consider using the hook instead?

I actually don't know what the hook is. Do you want to suggest language?

I removed the copying advice

pirj · 2024-10-16T05:35:21Z

lib/rubocop/cop/rspec/mixin/top_level_group.rb

@@ -29,6 +34,7 @@ def on_top_level_example_group(_node); end
        def on_top_level_group(_node); end

        def top_level_group?(node)
+          warn DEPRECATED_PRIVATE_METHOD_WARNING
          top_level_groups.include?(node)
        end



Something feels off about the node.children line.
I vaguely recall that a single-statement block and a multi-statement block’s AST are different (yuck!). Will the result be correct in both cases?
Like
foo { RSpec.describe { } } vs foo { RSpec.describe { }; RSpec.shared_examples { } }?

Unrelated to this PR, just an observation.
Might as well be a false alarm, and I apologise in advance.

Sounds like more specs are needed!

pirj · 2024-10-18T12:50:16Z

You’ll need to rebase to fix the failing config/docs CI check.

corsonknowles · 2024-10-21T06:19:12Z

Rebased and ready to go.

corsonknowles · 2024-10-25T12:31:25Z

@ydah or @bquorning Would either of you like to be the 2nd reviewer on this?

bquorning · 2024-10-25T21:26:31Z

lib/rubocop/cop/rspec/mixin/top_level_group.rb

@@ -7,6 +7,9 @@ module RSpec
      module TopLevelGroup
        extend RuboCop::NodePattern::Macros

+        DEPRECATED_PRIVATE_METHOD_WARNING =
+          'top_level_group? is deprecated and will be ' \
+          'removed in the next major version of Rubocop/RSpec.'


Suggested change

'removed in the next major version of Rubocop/RSpec.'

'removed in the next major version of rubocop_rspec.'

bquorning · 2024-10-25T21:28:31Z

lib/rubocop/cop/rspec/mixin/top_level_group.rb

        def top_level_group?(node)
+          warn DEPRECATED_PRIVATE_METHOD_WARNING


In RuboCop they use Rainbow to make yellow warnings. I’m a bit hesitant to add it as a dependency to our gem, but I think we should use the uplevel keyword argument that they also use:

Suggested change

warn DEPRECATED_PRIVATE_METHOD_WARNING

warn DEPRECATED_PRIVATE_METHOD_WARNING, uplevel: 1

That also prints the line that called the method, plus the word warning.

bquorning · 2024-10-25T21:31:16Z

spec/rubocop/cop/rspec/describe_class_spec.rb

+
+  context 'with mixin tests' do
+    describe '#top_level_group?' do
+      subject(:cop_with_top_level_group_mixin) do
+        described_class.new
+      end
+
+      it 'warns because it is deprecated' do
+        # rubocop:disable RSpec/SubjectStub
+        allow(cop_with_top_level_group_mixin).to receive(:warn)
+        allow(cop_with_top_level_group_mixin).to receive(:top_level_groups)
+          .and_return([])
+
+        cop_with_top_level_group_mixin.send(:top_level_group?, nil)
+        expect(cop_with_top_level_group_mixin).to have_received(:warn)
+        # rubocop:enable RSpec/SubjectStub
+      end
+    end
+  end


We can add a new file testing only the module you changed – and that way we can (almost) avoid stubbing. E.g.:

# spec/rubocop/cop/rspec/mixin/top_level_group_spec.rb RSpec.describe RuboCop::Cop::RSpec::TopLevelGroup do describe '#top_level_group?' do let(:stub_class) do Class.new do include RuboCop::Cop::RSpec::TopLevelGroup def initialize @top_level_groups = [] end def test_top_level_group top_level_group?(nil) end end end it 'warns because it is deprecated' do expect { stub_class.new.test_top_level_group }.to \ output(/warning: top_level_group\? is deprecated/).to_stderr end end end

In this PR, we add a deprecation message for top_level_group?, which has no callers in current Rubocop/RSpec. The specs for this change complete line coverage for this repo.

corsonknowles · 2024-10-25T23:24:13Z

Unfortunately after rebasing, there's a new straggler:

corsonknowles · 2024-10-25T23:25:05Z

This would have locked in coverage:

Update .simplecov #1971

But I can't chase a moving target at this speed.

…ethod

CHANGELOG.md

lib/rubocop/cop/rspec/mixin/top_level_group.rb

bquorning · 2024-10-26T08:54:13Z

Unfortunately after rebasing, there's a new straggler:

The straggler went away with #1985. I’ll rebase your other branch (#1971) and get it merged while we’re still at 100% ✨

corsonknowles requested a review from a team as a code owner October 14, 2024 22:47

corsonknowles force-pushed the test_and_deprecate_top_level_group_mixin_method branch from 6a1812f to d1de0e7 Compare October 14, 2024 22:50

This was referenced Oct 14, 2024

Complete line coverage for Metadata mixin #1970

Merged

Update .simplecov #1971

Merged

corsonknowles force-pushed the test_and_deprecate_top_level_group_mixin_method branch 2 times, most recently from fdcb6c8 to c99e4b6 Compare October 14, 2024 23:00

corsonknowles mentioned this pull request Oct 14, 2024

Complete branch coverage for ContextWording #1972

Merged

13 tasks

pirj reviewed Oct 16, 2024

View reviewed changes

corsonknowles force-pushed the test_and_deprecate_top_level_group_mixin_method branch from c99e4b6 to 3155c0e Compare October 18, 2024 02:34

corsonknowles force-pushed the test_and_deprecate_top_level_group_mixin_method branch from 3155c0e to 5726962 Compare October 18, 2024 13:13

pirj requested a review from a team October 18, 2024 13:39

bquorning mentioned this pull request Oct 25, 2024

Remove deprecated method #1986

Open

bquorning reviewed Oct 25, 2024

View reviewed changes

Deprecate top_level_group? and test it in a Cop class

b7d0877

In this PR, we add a deprecation message for top_level_group?, which has no callers in current Rubocop/RSpec. The specs for this change complete line coverage for this repo.

corsonknowles force-pushed the test_and_deprecate_top_level_group_mixin_method branch from e1a276b to b7d0877 Compare October 25, 2024 23:13

corsonknowles requested review from pirj and bquorning October 26, 2024 03:34

bquorning approved these changes Oct 26, 2024

View reviewed changes

Merge branch 'master' into test_and_deprecate_top_level_group_mixin_m…

a3eccd0

…ethod

bquorning force-pushed the test_and_deprecate_top_level_group_mixin_method branch from a0e69f8 to a3eccd0 Compare October 26, 2024 08:43

bquorning approved these changes Oct 26, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

lib/rubocop/cop/rspec/mixin/top_level_group.rb Outdated Show resolved Hide resolved

bquorning merged commit a7d140d into rubocop:master Oct 26, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate top_level_group? and test it in a Cop class #1976

Deprecate top_level_group? and test it in a Cop class #1976

corsonknowles commented Oct 14, 2024 •

edited

Loading

pirj Oct 16, 2024

corsonknowles Oct 18, 2024

corsonknowles Oct 25, 2024

bquorning Oct 25, 2024

pirj Oct 16, 2024

corsonknowles Oct 18, 2024

corsonknowles Oct 25, 2024

pirj Oct 16, 2024

corsonknowles Oct 18, 2024

pirj commented Oct 18, 2024

corsonknowles commented Oct 21, 2024

corsonknowles commented Oct 25, 2024

bquorning Oct 25, 2024

bquorning Oct 25, 2024

bquorning Oct 25, 2024

corsonknowles commented Oct 25, 2024

corsonknowles commented Oct 25, 2024

bquorning commented Oct 26, 2024

		@@ -29,6 +34,7 @@ def on_top_level_example_group(_node); end
		def on_top_level_group(_node); end

		def top_level_group?(node)

	'removed in the next major version of Rubocop/RSpec.'
	'removed in the next major version of rubocop_rspec.'

		def top_level_group?(node)
		warn DEPRECATED_PRIVATE_METHOD_WARNING

	warn DEPRECATED_PRIVATE_METHOD_WARNING
	warn DEPRECATED_PRIVATE_METHOD_WARNING, uplevel: 1

Deprecate top_level_group? and test it in a Cop class #1976

Deprecate top_level_group? and test it in a Cop class #1976

Conversation

corsonknowles commented Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj commented Oct 18, 2024

corsonknowles commented Oct 21, 2024

corsonknowles commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corsonknowles commented Oct 25, 2024

corsonknowles commented Oct 25, 2024

bquorning commented Oct 26, 2024

corsonknowles commented Oct 14, 2024 •

edited

Loading