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

Adds feature to use ULID in tests #444

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Adds feature to use ULID in tests #444

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b12971
Adds feature to use ULID instead of UUID in topic name and consumer g…
HolyKingCrusader May 27, 2024
640974c
split ULID id generation into a seperate function
HolyKingCrusader May 29, 2024
94113fa
changed func name
HolyKingCrusader Jun 1, 2024
247f2e9
added a field for setting function used in ID generation
HolyKingCrusader Jun 3, 2024
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
2 changes: 1 addition & 1 deletion pubsub/tests/bench_pubsub.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ type BenchmarkPubSubConstructor func(n int) (message.Publisher, message.Subscrib
// BenchSubscriber runs benchmark on a message Subscriber.
func BenchSubscriber(b *testing.B, pubSubConstructor BenchmarkPubSubConstructor) {
pub, sub := pubSubConstructor(b.N)
topicName := testTopicName(NewTestID())
topicName := testTopicName(NewTestID(false))

messages, err := sub.Subscribe(context.Background(), topicName)
if err != nil {
Expand Down
23 changes: 17 additions & 6 deletions pubsub/tests/test_pubsub.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,9 @@ type Features struct {
// NewSubscriberReceivesOldMessages should be set to true if messages are persisted even
// if they are already consumed (for example, like in Kafka).
NewSubscriberReceivesOldMessages bool

// UseULID should be set to true if ULID should be used instead of UUID
HolyKingCrusader marked this conversation as resolved.
Show resolved Hide resolved
UseULID bool
Copy link
Member

Choose a reason for hiding this comment

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

@HolyKingCrusader One more idea I had was that we could make it more open for future extensions. How about if there was a TestIDFunc (or similar) field which would default to NewTestID, but any other could be supplied? (Like NewTestULID())

Copy link
Author

Choose a reason for hiding this comment

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

@m110 Would it be used anywhere currently or just left for future use?

Copy link
Member

Choose a reason for hiding this comment

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

I guess in scenarios like those mentioned in #226. Similar to what you originally proposed. My point is to make it a bit more open for extension rather than just a UUID/ULID choice. It's not a big deal, though, so let me know your thoughts.

To be clear, I meant having a config field like TestIDGenerator func() TestID

Copy link
Author

Choose a reason for hiding this comment

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

I'll add it, doesn't seem to be too difficult

Copy link
Member

Choose a reason for hiding this comment

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

Hey @HolyKingCrusader, sorry about the delay here 😅

WIth the field you added to configure the function, the UseULID would be redundant now, correct?

Also, I think the default struct tag won't work like this out of the box, you might need to manually check if it's set and default to NewTestID otherwise.

}

// RunOnlyFastTests returns true if -short flag was provided -race was not provided.
Expand Down Expand Up @@ -151,7 +154,10 @@ func getTestName(testFunc interface{}) string {
type TestID string

// NewTestID returns a new unique TestID.
func NewTestID() TestID {
func NewTestID(useULID bool) TestID {
HolyKingCrusader marked this conversation as resolved.
Show resolved Hide resolved
if useULID {
return TestID(watermill.NewULID())
}
return TestID(watermill.NewUUID())
}

Expand All @@ -175,7 +181,7 @@ func runTest(
if parallel {
t.Parallel()
}
testID := NewTestID()
testID := NewTestID(features.UseULID)

t.Run(string(testID), func(t *testing.T) {
tCtx := TestContext{
Expand Down Expand Up @@ -804,8 +810,8 @@ func TestConsumerGroups(
}
totalMessagesCount := 50

group1 := generateConsumerGroup(t, pubSubConstructor, topicName)
group2 := generateConsumerGroup(t, pubSubConstructor, topicName)
group1 := generateConsumerGroup(t, pubSubConstructor, topicName, tCtx.Features.UseULID)
group2 := generateConsumerGroup(t, pubSubConstructor, topicName, tCtx.Features.UseULID)

messagesToPublish := PublishSimpleMessages(t, totalMessagesCount, publisherPub, topicName)

Expand Down Expand Up @@ -1225,8 +1231,13 @@ func closePubSub(t *testing.T, pub message.Publisher, sub message.Subscriber) {
require.NoError(t, err)
}

func generateConsumerGroup(t *testing.T, pubSubConstructor ConsumerGroupPubSubConstructor, topicName string) string {
groupName := "cg_" + watermill.NewUUID()
func generateConsumerGroup(t *testing.T, pubSubConstructor ConsumerGroupPubSubConstructor, topicName string, useULID bool) string {
groupName := "cg_"
if useULID {
groupName += watermill.NewULID()
} else {
groupName += watermill.NewUUID()
}

// create a pubsub to ensure that the consumer group exists
// for those providers that require subscription before publishing messages (e.g. Google Cloud PubSub)
Expand Down
Loading