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

Jump to bottom

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

Changes from all commits
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
23 changes: 19 additions & 4 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,12 @@ 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 for generating test IDs.
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.


// TestIDGenerator determines which function should be used for generating test IDs, NewTestID is used by default.
TestIDGenerator func() TestID `default:"NewTestID()"`
}

// RunOnlyFastTests returns true if -short flag was provided -race was not provided.
Expand Down Expand Up @@ -154,6 +160,10 @@ type TestID string
func NewTestID() TestID {
return TestID(watermill.NewUUID())
}
// NewTestULID returns a new unique TestID using ULID.
func NewTestULID() TestID {
return TestID(watermill.NewULID())
}

// TestContext is a collection of values that belong to a single test.
type TestContext struct {
Expand Down Expand Up @@ -804,8 +814,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 +1235,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