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

Adds feature to use ULID in tests #444

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

Conversation

HolyKingCrusader
Copy link

Adds feature to use ULID instead of UUID in topic name and consumer group.

Fixes #226's problem.

pubsub/tests/test_pubsub.go Outdated Show resolved Hide resolved
@m110
Copy link
Member

m110 commented May 28, 2024

Thanks for contributing @HolyKingCrusader! :)

pubsub/tests/test_pubsub.go Outdated Show resolved Hide resolved
@@ -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 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.

pubsub/tests/test_pubsub.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with topic and consumer group names in test suite
2 participants