rspec / rspec-rails

RSpec for Rails 6+
https://rspec.info
MIT License
5.18k stars 1.03k forks source link

undefined local variable or method `tagged_logger' during perform_enqueued_jobs #2545

Closed jkwuc89 closed 2 years ago

jkwuc89 commented 2 years ago

What Ruby, Rails and RSpec versions are you using?

Ruby version: 3.0.3p157 (2021-11-24 revision 3fb7d2cadc) [x86_64-darwin21] Rails version: 7.0.0 RSpec 3.10

Observed behaviour

Inside an rspec support module, my app is calling perform_enqueued_jobs. The spec in question that is using this support module is expecting this call to raise an exception.

When I attempt to run this spec, the following is raised:

got #<NameError: undefined local variable or method `tagged_logger'

When I debug into perform_enqueued_jobs, I am taken into _assert_nothing_raised_or_warn inside activesupport-7.0.0/lib/active_support/testing/assertions.rb, When debugging the block below, it shows e as having the expected exception.

        rescue Minitest::UnexpectedError => e
          if tagged_logger && tagged_logger.warn?
            warning = <<~MSG
              #{self.class} - #{name}: #{e.error.class} raised.
              If you expected this exception, use `assert_raises` as near to the code that raises as possible.
              Other block based assertions (e.g. `#{assertion}`) can be used, as long as `assert_raises` is inside their block.
            MSG
            tagged_logger.warn warning
          end

          raise
        end

Debugging further reveals that the if tagged_logger check above is causing the undefined error.

I reported this on Rails forum here and a member of the core team suggested the following:

"This seems to be a bug in RSpec that doesn’t implement the tagged_logger. Please report the issue in the RSpec issue tracker."

Expected behaviour

The spec should finish and allow to raise_error to check for the expected exception. This spec is working as expected in Rails 6.

pirj commented 2 years ago

Thanks for reporting. tagger_logger seems to have been introduced in this commit. Wondering if we should work this around by setting it to nil in our adapters code, or this should be fixed in Rails. Wondering where this tagged_logger is defined/initialized in Rails code for testing. Would you like to dig deeper in this, @jkwuc89 ?

jkwuc89 commented 2 years ago

I'll take a deeper dive into this and report what I find.

JonRowe commented 2 years ago

It feels like they are checking for a tagged logger by assuming it exists, they could easily fix this by checking its defined

jkwuc89 commented 2 years ago

Inside the Rails PR that added the block I included in the description above, one finds this comment which suggests checking if tagged_logger is defined. 🤔

https://github.com/rails/rails/pull/42459#discussion_r771836505

rafaelfranca commented 2 years ago

I think rspec-rails should be responsible for making sure the features of ActiveSupport::TestCase it uses works with RSpec, nor Rails that should check if its own API is existent.

pirj commented 2 years ago

Thanks for the hints @rafaelfranca, @ghiculescu.

@jkwuc89 Does it fix the issue if you add the following to rspec-rails' lib/rspec/rails/example/rails_example_group.rb?

        extend ActiveSupport::Concern
+       include ActiveSupport::Testing::TaggedLogging
        include RSpec::Rails::SetupAndTeardownAdapter
westonganger commented 2 years ago

Here is a quick monkey patch using @pirj suggested solution

# spec/rspec_helper.rb

if Rails::VERSION::MAJOR >= 7
  require 'rspec/rails/version'

  if Gem::Version.create(RSpec::Rails::Version::STRING) < Gem::Version.create("5.1.0")
    RSpec::Rails::RailsExampleGroup.module_eval do
      include ActiveSupport::Testing::TaggedLogging
    end
  end
end
jkwuc89 commented 2 years ago

Adding the following to spec/rails_helper.rb inside my Rails project allows tagged_logger to get defined which fixes the original undefined error in the description above.

if Rails::VERSION::MAJOR >= 7
  include ActiveSupport::Testing::TaggedLogging
end

Unfortunately, it causes another issue when I run the spec in question. When _assert_nothing_raised_or_warn is called inside activesupport-7.0.0/lib/active_support/testing/assertions.rb as part of handling an exception during perform_enqueued_jobs, it causes the spec to fail with the following because _assert_nothing_raised_or_warn uses name inside its definition.

expected Workflow::SellerAuthorization::MissingSellerAuthorizationError, got #<RSpec::Core::ExampleGroup::WrongScopeError: `name` is not available from within an example (e.g. an...ore`, `let`, etc). It is only available on an example group (e.g. a `describe` or `context` block).> with backtrace:
  # ./spec/support/events/async_event_processor.rb:6:in `publish_event_inline'
  # ./spec/models/workflow/seller_authorization_spec.rb:668:in `block (3 levels) in <top (required)>'
  # ./spec/models/workflow/seller_authorization_spec.rb:699:in `block (6 levels) in <top (required)>'
  # ./spec/models/workflow/seller_authorization_spec.rb:699:in `block (5 levels) in <top (required)>'
  # ./spec/spec_helper.rb:20:in `block (2 levels) in <top (required)>'
xxx commented 2 years ago

I'm using the following crappy patch, as including the module into RSpec::Rails::RailsExampleGroup did not do anything.

Also defined a dummy name method (sigh), to resolve the subsequent issue that @jkwuc89 raises just above (which this also runs into)

if Rails::VERSION::MAJOR >= 7
  require 'rspec/rails/version'

  RSpec::Core::ExampleGroup.module_eval do
    include ActiveSupport::Testing::TaggedLogging

    def name
      'foobar'
    end
  end
end
JonRowe commented 2 years ago

PRs would be accepted to solve this, RailsExampleGroup should conditionally (as in, if on Rails 7) include this module, and ideally we'd want a snippet testing it.

JonRowe commented 2 years ago

This was fixed in 6.0.0 and further improved in 6.0.1

ricardopacheco commented 1 year ago

@JonRowe the problem still persists, version 6.0.1

JonRowe commented 1 year ago

Please provide a reproduction snippet.

joemsak commented 1 year ago

I am getting this error in 6.0.1 with this test:

it "test description" do
  expect {
    perform_enqueued_jobs do
      # failing LOC here, due to raised exception (uncaught, unexpected)
    end
  }.to change { ActionMailer::Base.deliveries.size }.by(2)
end

I get just enough of the actual backtrace in the stdout to then pry in and figure out the exception / failure myself but yeah, ultimately it's slowing me down

riffraff commented 1 year ago

I also see the same error with 6.0.1, my code looks similar

      specify do
        expect { perform_enqueued_jobs { dispatch } }
          .to not_change(ActionMailer::Base.deliveries, :size)
          .and not_raise_error

Ruby 3.2.1, rails 7.0.4

pirj commented 1 year ago

Does the monkey patch mentioned above fix it for you @riffraff @joemsak ?

It would be very helpful if you could provide a runnable snippet to reproduce the issue. doc, examples.

Just for the reference, related PRs: #2587, #2625.

bigxiang commented 1 year ago

@pirj I just saw this error in 6.0.1, and the monkey patch above https://github.com/rspec/rspec-rails/issues/2545#issuecomment-1029539971 can fix the issue.

However, I find I missed type: :job in my spec. If I add it, everything works fine. I think it's because the error is raised by RSpec::Core::ExampleGroup if I don't add type: :job, so the monkey patch works. When running a spec with type: :job, RSpec::Rails::RailsExampleGroup is included, so the fix works. Does it make sense?

pirj commented 1 year ago

If it’s not a Rails example group (no ‘type:’ set), why it is expected to include the code introduced in #2587 ? I would refrain from including this to non-Rails example groups. Am I missing something?

brianaj commented 1 year ago

Still see this issue and https://github.com/rspec/rspec-rails/issues/2545#issuecomment-1029539971 helps resolve it but would like to understand why https://github.com/rspec/rspec-rails/pull/2587 doesn't fix things?

pirj commented 1 year ago

Because this is only for Rails example groups. But the patch you refer to does patch it for all example groups. Do you add proper metadata to your Raisl-related example groups?

sherryptk commented 12 months ago

I'm running this code:

describe "#request_token" do it "returns a successful response" do post :request_token, params: { username: 'XXX', password: 'XXX' } expect(response).to have_http_status(:success) end end

and am getting this error:

=> There was an error building fixtures => #<ArgumentError: wrong number of arguments (given 0, expected 1+)>

/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/bundler/gems/fixture_builder-7a042cc63699/lib/fixture_builder/namer.rb:16:in name' /.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/bundler/gems/fixture_builder-7a042cc63699/lib/fixture_builder/delegations.rb:16:inname' /.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7.2/lib/active_support/testing/assertions.rb:254:in rescue in _assert_nothing_raised_or_warn' /.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.7.2/lib/active_support/testing/assertions.rb:249:in_assert_nothing_raised_or_warn'

I got past the first "tagged logger" error by adding include ActiveSupport::Testing::TaggedLogging, however I'm seeing this afterward and haven't seen anything addressing it. Adding def name did not work for me.

pirj commented 12 months ago

I see some ‘fixture_builder’ gem in the log. And i see you’ve filed a bug there, too https://github.com/rdy/fixture_builder/issues/70

Can you check if this branch would fix your issue? https://github.com/rdy/fixture_builder/pull/68

sebaherrera07 commented 5 days ago

I'm still seeing this issue with Ruby (3.3.5), Rails (7.1.4), rspec (3.13.0), rspec-rails (7.0.1). To me it's pointing here:

NameError:
       undefined local variable or method `tagged_logger' for an instance of FactoryBot::SyntaxRunner

But don't think is has to do with FactoryBot, it's seems is just because I have my perform_enqueued_jobs in a factory.

Any news on this cc @jkwuc89 @pirj @JonRowe

pirj commented 5 days ago

@sebaherrera07 Can you please:

JonRowe commented 5 days ago

I don't know, that looks to me like the factory needs to have the tagger logger included and thats out of scope for us.