-
Notifications
You must be signed in to change notification settings - Fork 232
fix: make send subject optional via context #1143
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
Conversation
1d08816
to
9b37c1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, can you please squash and clean up your commits?
9b37c1a
to
1c14031
Compare
Get we get a testcase? |
|
Yeah sorry for the code base...I created a small unit test to verify the behavior, patch below diff --git a/protocol/nats_jetstream/v3/protocol_test.go b/protocol/nats_jetstream/v3/protocol_test.go
new file mode 100644
index 0000000..b857a94
--- /dev/null
+++ b/protocol/nats_jetstream/v3/protocol_test.go
@@ -0,0 +1,85 @@
+/*
+ Copyright 2024 The CloudEvents Authors
+ SPDX-License-Identifier: Apache-2.0
+*/
+
+package nats_jetstream
+
+import (
+ "context"
+ "testing"
+
+ "github.com/cloudevents/sdk-go/v2/test"
+ "github.com/nats-io/nats.go"
+ "github.com/nats-io/nats.go/jetstream"
+)
+
+// mockJetStream implements jetstream.JetStream interface for testing
+type mockJetStream struct {
+ streamNameBySubjectFunc func(ctx context.Context, subject string) (string, error)
+ publishMsgFunc func(ctx context.Context, msg *nats.Msg, opts ...jetstream.PublishOpt) (*jetstream.PubAck, error)
+ jetstream.JetStream
+}
+
+func (m *mockJetStream) StreamNameBySubject(ctx context.Context, subject string) (string, error) {
+ return m.streamNameBySubjectFunc(ctx, subject)
+}
+
+func (m *mockJetStream) PublishMsg(ctx context.Context, msg *nats.Msg, opts ...jetstream.PublishOpt) (*jetstream.PubAck, error) {
+ return m.publishMsgFunc(ctx, msg, opts...)
+}
+
+func TestSend(t *testing.T) {
+ tests := []struct {
+ name string
+ sendSubject string
+ ctx context.Context
+ wantSubject string
+ }{
+ {
+ name: "using p.sendSubject",
+ sendSubject: "test.subject",
+ ctx: context.Background(),
+ wantSubject: "test.subject",
+ },
+ {
+ name: "using WithSubject",
+ sendSubject: "",
+ ctx: WithSubject(context.Background(), "test.ctxSubject"),
+ wantSubject: "test.ctxSubject",
+ },
+ {
+ name: "using WithSubject and p.sendSubject",
+ sendSubject: "test.subject",
+ ctx: WithSubject(context.Background(), "test.ctxSubject"),
+ wantSubject: "test.ctxSubject",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ mockJS := &mockJetStream{
+ streamNameBySubjectFunc: func(ctx context.Context, subject string) (string, error) {
+ if subject != tt.wantSubject {
+ t.Errorf("unexpected subject: got %s, want %s", subject, tt.wantSubject)
+ }
+ return "test-stream", nil
+ },
+ publishMsgFunc: func(ctx context.Context, msg *nats.Msg, opts ...jetstream.PublishOpt) (*jetstream.PubAck, error) {
+ if msg.Subject != tt.wantSubject {
+ t.Errorf("unexpected subject in publish: got %s, want %s", msg.Subject, tt.wantSubject)
+ }
+ return nil, nil
+ },
+ }
+
+ p := &Protocol{
+ jetStream: mockJS,
+ sendSubject: tt.sendSubject,
+ }
+
+ msg := test.FullMessage()
+ _ = p.Send(tt.ctx, msg)
+ })
+ }
+} |
@kmpm mind just copy and pasting the test so we can close this PR? |
@embano1 , I will fix it. Life got in the way. Thanks. |
aa2bf56
to
7986922
Compare
Thank you! I rebased onto the most recent commit. Can you please squash the test commit into one final commit? Should also fix the DCO warning (you need to |
added tests from @embano1 Signed-off-by: Peter Magnusson <me@kmpm.se>
7986922
to
9318872
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I had some issues with a single subject regardless of event. Added a function to add an alternate subject via context.
This will also possibly close #1115