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.

Sign up for GitHub

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

Do not mark classes that inherit from ActiveJob::TestCase as dead #592

Closed
wants to merge 1 commit into from
Closed

Do not mark classes that inherit from ActiveJob::TestCase as dead #592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/spoom/deadcode/plugins/active_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ module Spoom
module Deadcode
module Plugins
class ActiveJob < Base
ignore_classes_inheriting_from("ActiveJob::TestCase")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't ActiveJob::TestCase a subclass of ActiveSupport::TestCase (https://github.com/rails/rails/blob/main/activejob/lib/active_job/test_case.rb#L6)? The ActiveSupport plugin should be catching this so there is another kind of bug at play here. I don't think this is the right fix.

Copy link
Collaborator

@Morriar Morriar Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I think I know what is happening here. We only index .rb files in CodeDB: https://github.com/Shopify/code-db/blob/main/app/jobs/jobs/project/spoom_deadcode.rb#L9. The relationship between ActiveJob::TestCase and ActiveSupport::TestCase may only exist in the RBI files so the Spoom Model doesn't see it...

I wonder what would be the impact of also adding .rbi files to the Model (not the dead code Index though as we don't want to report anything about RBI files). We should time it to see how much slower it makes the analysis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't ActiveJob::TestCase a subclass of ActiveSupport::TestCase

It definitely is but since it wasn't working I just assumed that "for reasons" we only look at the direct parent without trying to analyze the whole inheritance chain.

But if we expect ignore_classes_inheriting_from to apply to the whole chain then I totally agree we should be looking into keeping only ActiveSupport::TestCase as ignored parent and fixing the lookup instead

I wonder what would be the impact of also adding .rbi files to the Model

I can allocate some time to do that but since we are experiencing an issue right now and current version has a bug how would you feel about either merging this PR with explicit ignore of ActiveJob::TestCase or perhaps temporarily removing changes to Minitest plugin (bringing back ignore of *Test classes

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can merge this. Though, since you know Rails can you ensure that we not missing other subclasses of ActiveSupport::TestCase?

I'll handle the RBI indexing exploration 👍

ignore_classes_named("ApplicationJob")
ignore_methods_named("perform", "build_enumerator", "each_iteration")
end
Expand Down
9 changes: 9 additions & 0 deletions test/spoom/deadcode/plugins/active_job_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ def perform; end
)
end

def test_ignores_activejob_testcase_class_definition
@project.write!("test/foo_test.rb", <<~RB)
class FooTest < ActiveJob::TestCase
end
RB

assert_ignored(index_with_plugins, "FooTest")
end

private

sig { returns(Deadcode::Index) }
Expand Down
Loading