From 525e8203df85d265d9b419f72f5a3c84b4ecbe31 Mon Sep 17 00:00:00 2001 From: Juniper Alanna <201364921+juniper-shopify@users.noreply.github.com> Date: Tue, 2 Jun 2026 20:36:37 -0400 Subject: [PATCH] Add cop to enforce no reopening classes in test class hierarchy --- .rubocop.yml | 7 +- .../cop/roast/no_test_class_nesting.rb | 126 ++++++++ internal/rubocop/rubocop-roast.yml | 6 + .../cop/roast/no_test_class_nesting_test.rb | 305 ++++++++++++++++++ 4 files changed, 443 insertions(+), 1 deletion(-) create mode 100644 internal/rubocop/cop/roast/no_test_class_nesting.rb create mode 100644 internal/rubocop/rubocop-roast.yml create mode 100644 test/rubocop/cop/roast/no_test_class_nesting_test.rb diff --git a/.rubocop.yml b/.rubocop.yml index 262ce8d9..93b54833 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,4 +1,9 @@ -inherit_from: .rubocop_todo.yml +inherit_from: + - .rubocop_todo.yml + - internal/rubocop/rubocop-roast.yml + +require: + - ./internal/rubocop/cop/roast/no_test_class_nesting plugins: - rubocop-sorbet diff --git a/internal/rubocop/cop/roast/no_test_class_nesting.rb b/internal/rubocop/cop/roast/no_test_class_nesting.rb new file mode 100644 index 00000000..d4b44db9 --- /dev/null +++ b/internal/rubocop/cop/roast/no_test_class_nesting.rb @@ -0,0 +1,126 @@ +# typed: false +# frozen_string_literal: true + +module RuboCop + module Cop + module Roast + # Prevents nesting classes or modules inside reopened (non-test) class + # definitions in test files. + # + # When a class is reopened in a test file (e.g., `class Agent < Cog`) and + # contains nested class or module definitions, IDE test runners like + # RubyMine fail to discover test suites. Use `::` scoping instead. + # + # The cop walks the entire subtree of the offending class, so deeply + # nested structures (e.g., `class Agent < Cog; module Providers; ...`) + # are caught even when the nested definitions are not direct children. + # + # Classes nested inside test classes are exempt — helper stubs and + # fixtures defined inside a test suite are perfectly fine. + # + # @example Bad — reopened class with nested module + # class Agent < Cog + # module Providers + # class Claude::MessageTest < ActiveSupport::TestCase + # # ... + # end + # end + # end + # + # @example Bad — reopened class with nested test class + # class Agent < Cog + # class ConfigTest < ActiveSupport::TestCase + # # ... + # end + # end + # + # @example Good — :: scoping, no class reopening + # module Agent::Providers + # class Claude::MessageTest < ActiveSupport::TestCase + # # ... + # end + # end + # + # @example Good — :: scoped test class + # class Agent::ConfigTest < ActiveSupport::TestCase + # # ... + # end + # + # @example Good — helper class inside a test class + # class Agent::OutputTest < ActiveSupport::TestCase + # class FakeAdapter + # def call; end + # end + # end + # + class NoTestClassNesting < Base + MSG = "Do not nest classes or modules inside reopened class `%s` in test files. " \ + "Use `::` scoping instead (e.g., `class %s::Nested` or `module %s::Nested`)." + + # @!method test_base_class?(node) + def_node_matcher :test_base_class?, <<~PATTERN + { + (const (const {nil? cbase} :ActiveSupport) :TestCase) + (const (const {nil? cbase} :Minitest) :Test) + (const {nil? cbase} :Minitest) + } + PATTERN + + def on_class(node) + # Test classes are allowed to contain nested definitions (helpers, stubs) + return if test_class?(node) + + # Classes nested inside a test class are helpers — leave them alone + return if inside_test_class?(node) + + # Flag if this non-test class contains any nested class or module + return unless contains_nested_definitions?(node) + + message = format(MSG, parent: node.identifier.const_name) + add_offense(node.loc.keyword.join(node.identifier.source_range), message: message) + end + + private + + def test_class?(node) + node.parent_class && test_base_class?(node.parent_class) + end + + # Returns true if any ancestor of +node+ is a test class. + def inside_test_class?(node) + current = node.parent + while current + return true if current.class_type? && test_class?(current) + + current = current.parent + end + false + end + + # Returns true if +node+ contains any nested class or module definition + # at any depth, excluding those sheltered inside an intermediate test class. + def contains_nested_definitions?(node) + node.each_descendant(:class, :module) do |descendant| + next if sheltered_by_test_class?(descendant, node) + + return true + end + false + end + + # Returns true if there is a test class between +descendant+ and +stop_at+ + # in the ancestor chain — meaning the descendant is a helper inside a test + # class and should not count as a problematic nested definition. + def sheltered_by_test_class?(descendant, stop_at) + current = descendant.parent + while current && current != stop_at + return true if current.class_type? && test_class?(current) + + current = current.parent + end + false + end + end + end + end +end diff --git a/internal/rubocop/rubocop-roast.yml b/internal/rubocop/rubocop-roast.yml new file mode 100644 index 00000000..2d8cf6fe --- /dev/null +++ b/internal/rubocop/rubocop-roast.yml @@ -0,0 +1,6 @@ +Roast/NoTestClassNesting: + Description: "Prevents nesting test classes inside reopened production classes. Use :: scoping instead." + Enabled: true + VersionAdded: "0.1.0" + Include: + - "test/**/*.rb" diff --git a/test/rubocop/cop/roast/no_test_class_nesting_test.rb b/test/rubocop/cop/roast/no_test_class_nesting_test.rb new file mode 100644 index 00000000..1729f508 --- /dev/null +++ b/test/rubocop/cop/roast/no_test_class_nesting_test.rb @@ -0,0 +1,305 @@ +# frozen_string_literal: true + +require "test_helper" +require "rubocop" +require_relative "../../../../internal/rubocop/cop/roast/no_test_class_nesting" + +module RuboCop + module Cop + module Roast + class NoTestClassNestingTest < ActiveSupport::TestCase + def setup + config = RuboCop::Config.new("Roast/NoTestClassNesting" => { "Enabled" => true }) + @cop = NoTestClassNesting.new(config) + end + + # === Offense cases: direct nesting === + + test "flags test class nested directly inside reopened class with superclass" do + offenses = investigate(<<~RUBY) + class Agent < Cog + class ConfigTest < ActiveSupport::TestCase + end + end + RUBY + + assert_equal 1, offenses.size + assert_includes offenses.first.message, "reopened class `Agent`" + end + + test "flags test class nested inside reopened class without superclass" do + offenses = investigate(<<~RUBY) + class Cog + class ConfigTest < ActiveSupport::TestCase + end + end + RUBY + + assert_equal 1, offenses.size + assert_includes offenses.first.message, "reopened class `Cog`" + end + + test "flags when multiple test classes are nested inside one wrapper" do + offenses = investigate(<<~RUBY) + class Agent < Cog + class ConfigTest < ActiveSupport::TestCase + end + + class InputTest < ActiveSupport::TestCase + end + end + RUBY + + # One offense for the wrapper class + assert_equal 1, offenses.size + end + + test "flags Minitest::Test as a test base class" do + offenses = investigate(<<~RUBY) + class Agent < Cog + class ConfigTest < Minitest::Test + end + end + RUBY + + assert_equal 1, offenses.size + end + + # === Offense cases: nested modules === + + test "flags module nested inside reopened class" do + offenses = investigate(<<~RUBY) + class Agent < Cog + module Providers + end + end + RUBY + + assert_equal 1, offenses.size + assert_includes offenses.first.message, "reopened class `Agent`" + end + + test "flags non-test class nested inside reopened class" do + offenses = investigate(<<~RUBY) + class Agent < Cog + class Config + def initialize; end + end + end + RUBY + + assert_equal 1, offenses.size + assert_includes offenses.first.message, "reopened class `Agent`" + end + + # === Offense cases: deep nesting === + + test "flags deeply nested module inside reopened class" do + offenses = investigate(<<~RUBY) + class Agent < Cog + module Providers + class Claude::MessageTest < ActiveSupport::TestCase + end + end + end + RUBY + + # Agent is flagged because it contains nested module Providers + assert_equal 1, offenses.size + assert_includes offenses.first.message, "reopened class `Agent`" + end + + test "flags reopened class with deeply nested class chain" do + offenses = investigate(<<~RUBY) + class Agent < Cog + module Providers + class Claude < Provider + class MessageTest < ActiveSupport::TestCase + end + end + end + end + RUBY + + # Agent is flagged (contains module Providers) + # Claude is also flagged (non-test class containing MessageTest) + assert_equal 2, offenses.size + parent_names = offenses.map { |o| o.message[/`(\w+)`/, 1] } + assert_includes parent_names, "Agent" + assert_includes parent_names, "Claude" + end + + test "flags wrapper class inside modules" do + offenses = investigate(<<~RUBY) + module Roast + module Cogs + class Agent < Cog + class ConfigTest < ActiveSupport::TestCase + end + end + end + end + RUBY + + assert_equal 1, offenses.size + end + + test "offense highlights class keyword and name" do + offenses = investigate(<<~RUBY) + class Agent < Cog + module Providers + end + end + RUBY + + assert_equal 1, offenses.first.line + assert_equal 0, offenses.first.column + end + + # === No-offense cases: :: scoping === + + test "allows test class with :: scoping" do + offenses = investigate(<<~RUBY) + class Agent::ConfigTest < ActiveSupport::TestCase + end + RUBY + + assert_empty offenses + end + + test "allows test class with :: scoping inside modules" do + offenses = investigate(<<~RUBY) + module Roast + module Cogs + class Agent::ConfigTest < ActiveSupport::TestCase + end + end + end + RUBY + + assert_empty offenses + end + + test "allows :: scoped module with test class inside" do + offenses = investigate(<<~RUBY) + module Roast + module Cogs + module Agent::Providers + class Claude::MessageTest < ActiveSupport::TestCase + end + end + end + end + RUBY + + assert_empty offenses + end + + test "allows deeply :: scoped module" do + offenses = investigate(<<~RUBY) + module Roast + module Cogs + module Agent::Providers::Claude::Messages + class TextMessageTest < ActiveSupport::TestCase + end + end + end + end + RUBY + + assert_empty offenses + end + + # === No-offense cases: helpers inside test classes === + + test "allows helper classes nested inside test classes" do + offenses = investigate(<<~RUBY) + class Agent::OutputTest < ActiveSupport::TestCase + class FakeAdapter + def call; end + end + end + RUBY + + assert_empty offenses + end + + test "allows helper subclass of production class inside test class" do + offenses = investigate(<<~RUBY) + class ConfigManagerTest < ActiveSupport::TestCase + class TestCogConfig < Cog::Config + field :timeout, 30 + end + + class TestCog < Cog + class Config < TestCogConfig; end + + def execute(_input) + raise NotImplementedError + end + end + end + RUBY + + assert_empty offenses + end + + test "allows Struct inside test class" do + offenses = investigate(<<~RUBY) + class Cmd::OutputTest < ActiveSupport::TestCase + ProcessStatus = Struct.new(:exitstatus, :success) + end + RUBY + + assert_empty offenses + end + + # === No-offense cases: other === + + test "allows multiple test classes at module level" do + offenses = investigate(<<~RUBY) + module Roast + module Cogs + class Cmd::ConfigTest < ActiveSupport::TestCase + end + + class Cmd::InputTest < ActiveSupport::TestCase + end + end + end + RUBY + + assert_empty offenses + end + + test "allows single test class without wrapper" do + offenses = investigate(<<~RUBY) + class SimpleTest < ActiveSupport::TestCase + end + RUBY + + assert_empty offenses + end + + test "allows empty reopened class (no nested definitions)" do + offenses = investigate(<<~RUBY) + class Agent < Cog + def some_method; end + end + RUBY + + assert_empty offenses + end + + private + + def investigate(source_code) + source = RuboCop::ProcessedSource.new(source_code, RUBY_VERSION.to_f) + commissioner = RuboCop::Cop::Commissioner.new([@cop]) + result = commissioner.investigate(source) + result.offenses + end + end + end + end +end