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

ApplicationConsumer testing #23

Open
ojab opened this issue Nov 2, 2022 · 6 comments
Open

ApplicationConsumer testing #23

ojab opened this issue Nov 2, 2022 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@ojab
Copy link

ojab commented Nov 2, 2022

We have something like

class ApplicationConsumer < Karafka::BaseConsumer
  def self.on_unknown(message)
    
  end

  def consume
    messages.each { |message| consume_single(message) }
  end

  def consume_single(message)
    return self.class.on_unknown(message) unless validate(message)

    MyApp::Metrics.with_context(**metrics_context(message)) do
      process(message)
    end
  end

with a bunch of code handling unknown messages, errors and adding some context for metrics and error reporting in AppicationConsumer and all other consumers inherited from this one.
In specs it's tested (or was tested in karafka-1) like

RSpec.describe ApplicationConsumer do
   let(:consumer) do
      Class.new(described_class) do
        def initialize; end # rubocop:disable Style/RedundantInitialize required to avoid exception with different arity
      end.new
    end
end

and with karafka-2 it fails because karafka-testing implies that consumer has topic set, for example here and here.
It could be workarounded by

before
  consumer_group = Karafka::Routing::ConsumerGroup.new('foo')  
  consumer.topic = Karafka::Routing::Topic.new('foo', consumer_group)            
end

but would be good to retain the ability to test topic-less consumers or at least simplify/document how consumer group should be created.

@mensfeld
Copy link
Member

mensfeld commented Nov 2, 2022

So, basically you want to test abstract consumers right?

@ojab
Copy link
Author

ojab commented Nov 2, 2022

Yep!

@mensfeld
Copy link
Member

mensfeld commented Nov 2, 2022

Now we could get into an argument whether or not you should test abstract classes or their applications ;)

I will think what to do with this in the upcoming days though.

@mensfeld mensfeld added the enhancement New feature or request label Nov 2, 2022
@mensfeld mensfeld self-assigned this Nov 2, 2022
@ojab
Copy link
Author

ojab commented Nov 2, 2022

Overall adding documentation would be fine, I guess, since at least in this particular project it's tested in a single spec and adding consumer.topic is not a big deal.

@mensfeld
Copy link
Member

mensfeld commented Nov 3, 2022

@ojab would you mind expanding the docs? They are a wiki: https://github.com/karafka/wiki/blob/master/Testing.md so you can just PR.

@mensfeld mensfeld transferred this issue from karafka/karafka-testing Nov 4, 2022
@mensfeld
Copy link
Member

mensfeld commented Nov 4, 2022

Moved to wiki as it is for docs expansion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

2 participants