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

karafka helpers initialize described_class, they shouldn't #106

Closed
ojab opened this issue Oct 20, 2022 · 8 comments
Closed

karafka helpers initialize described_class, they shouldn't #106

ojab opened this issue Oct 20, 2022 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@ojab
Copy link
Contributor

ojab commented Oct 20, 2022

Consider the following innocent spec:

class Xxx
  def initialize(foo:)
  end
end

RSpec.describe Xxx do
  include Karafka::Testing::RSpec::Helpers

  specify do
    expect { Karafka.producer.produce_sync(topic: 'foo', payload: 'bar') }
      .to change(karafka.produced_messages, :size).by(1)
  end
end

#=>

Failures:

  1) Xxx is expected to change `Array#size` by 1
     Failure/Error:
       def initialize(foo:)
       end
     
     ArgumentError:
       missing keyword: :foo
     # ./spec/xxx_spec.rb:2:in `initialize'
     # …/rspec-core-3.11.0/lib/rspec/core/memoized_helpers.rb:60:in `new'
…
     # …/karafka-testing-2.0.5/lib/karafka/testing/rspec/helpers.rb:91:in `_karafka_add_message_to_consumer_if_needed'


https://github.com/karafka/karafka-testing/blob/master/lib/karafka/testing/rspec/helpers.rb#L87-L94

        def _karafka_add_message_to_consumer_if_needed(message)
          # We're interested in adding message to subject only when it is a consumer
          # Users may want to test other things (models producing messages for example) and in
          # their case subject will not be a consumer
          return unless subject.is_a?(Karafka::BaseConsumer)
          # We target to the consumer only messages that were produced to it, since specs may also
          # produce other messages targeting other topics
          return unless message[:topic] == subject.topic.name

https://github.com/rspec/rspec-core/blob/main/lib/rspec/core/memoized_helpers.rb#L57-L62

      def subject
        __memoized.fetch_or_store(:subject) do
          described = described_class || self.class.metadata.fetch(:description_args).first
          Class === described ? described.new : described
        end
      end

so by calling subject we're trying to initialize the class that requires some arguments.

@NotGrm
Copy link

NotGrm commented Oct 21, 2022

I have an error that is different, but the cause is the same as the one described by @ojab

I am testing a API request in my Rails app and I have defined a subject to run it,

  subject(:action) { get "/books/#{book_id}/position" }

During the request processing, we are producing a Kafka event and since I have added the Karafka.produce_async call and get a stack level too deep error because when the subject is called it produces an event, but the Karafka helpers are calling subject (which is still not memorized at this step), which call subject again, which produce a new Kafka event, and so on ...

For the moment the quick fix I found is to change my subject declaration to

  let(:action) { get "/books/#{book_id}/position" }

@mensfeld mensfeld self-assigned this Oct 22, 2022
@mensfeld mensfeld added the bug Something isn't working label Oct 22, 2022
@mensfeld
Copy link
Member

mensfeld commented Oct 24, 2022

Let me try to reproduce both...

ok I can reproduce the first one

@mensfeld
Copy link
Member

Ok @ojab @NotGrm my suggestion here would be for me to:

  1. Replace the subject reference with consumer reference.
  2. Not proxy anything to consumer unless let(:consumer) exists
  3. Not proxy anything unless consumer is a Karafka consumer

This would mean we force a naming convention where consumer needs to be built with a let/subject(:consumer) { ... } but this is already recommended and this convention is in all the examples.

I do not see any other way and I made a POC here: #108

Feel free to also check if it solves your problems. It works for me

@NotGrm
Copy link

NotGrm commented Oct 24, 2022

I agree with you, there seem to not be other ways than using a named reference instead of an implicit subject.

But I'm wondering if it could not be possible to define this consumer reference automatically based on the described class properties 🤔 I'm not used to RSpec metaprog but maybe we could have something like

  1. Does a consumer reference exist? If yes, use it and test if it is a Karafka consumer
  2. Otherwise look at described_class ancestors (or file path 🤷 ) if includes the Karafka base consumer Bingo we can create the missing consumer reference based on subject value

@mensfeld
Copy link
Member

But I'm wondering if it could not be possible to define this consumer reference automatically based on the described class properties

sounds like too much 🪄 to me tbh. Checking descendants, figuring things and building instead of just invoking consumer creation? Isn't it too much?

I think it may be worth considering in a separate change though. Would you mind doing two things for me?

  1. Checking that his PR solves your problem
  2. Creating a separate issue for this "figuring out what to build"?

This fix does not contradict the other so we should be fine with patch release if solves both of your problems.

@merciremi
Copy link

merciremi commented Oct 26, 2022

👋 Howdy folks

I've had the same issue as @NotGrm and I can confirm that trying this PR solves the stack level too deep and another issue I had where karafka-testing was trying to look-up topic.name on my subject. 👍

@mensfeld
Copy link
Member

closing. It will be released up to 15 minutes.

@mensfeld
Copy link
Member

🚀 karafka-testing 2.0.5 has been released:
Replace the subject reference with named consumer reference.
Do not forward sent messages to consumer unless it's a Karafka consumer.

https://rubygems.org/gems/karafka-testing/versions/2.0.6
https://my.diffend.io/gems/karafka-testing/2.0.5/2.0.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

4 participants