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

chore(test): use testcontainers modules #1164

Merged
merged 3 commits into from Sep 22, 2023
Merged

chore(test): use testcontainers modules #1164

merged 3 commits into from Sep 22, 2023

Conversation

0xERR0R
Copy link
Owner

@0xERR0R 0xERR0R commented Sep 20, 2023

testcontainers provides predefined containers for major databases. This PR replaces mariadb, postgeres and redis with testcontainers modules.

@0xERR0R 0xERR0R added the 🧰 technical debts Technical debts, refactoring label Sep 20, 2023
@0xERR0R 0xERR0R added this to the v0.23 milestone Sep 20, 2023
@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Patch has no changes to coverable lines.

📢 Thoughts on this report? Let us know!.

@0xERR0R 0xERR0R marked this pull request as draft September 20, 2023 08:32
@0xERR0R 0xERR0R force-pushed the testcontainers-module branch 2 times, most recently from 12e221b to 618cc87 Compare September 20, 2023 09:13
@0xERR0R 0xERR0R marked this pull request as ready for review September 20, 2023 09:27
@0xERR0R 0xERR0R enabled auto-merge (rebase) September 20, 2023 15:26
@0xERR0R 0xERR0R enabled auto-merge (squash) September 21, 2023 14:22
Comment on lines +129 to +132
testcontainers.WithWaitStrategy(
wait.ForLog("database system is ready to accept connections").
WithOccurrence(waitLogOccurrence).
WithStartupTimeout(startupTimeout)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was wondering why this is the only one using WithWaitStrategy and from looking at testcontainers code, the others have one built-in.
Might be worth making an issue/PR to add a built-in one for postgres too since the others also use log matching, so there's no reason this couldn't be built-in.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, I have also noticed that. In official example for postgres (https://golang.testcontainers.org/modules/postgres/) is a wait strategy defined. In redis and mariaDB not.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice find, glad they're thinking of it.

@0xERR0R 0xERR0R merged commit bcff170 into main Sep 22, 2023
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the testcontainers-module branch September 22, 2023 14:35
ExposedPorts: []string{"6379/tcp"},
NetworkAliases: map[string][]string{NetworkName: {"redis"}},
WaitingFor: wait.ForExposedPort(),
func WithNetwork(network string) testcontainers.CustomizeRequestOption {

Choose a reason for hiding this comment

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

FYI we have just added a WithNetwork option here: testcontainers/testcontainers-go#1894

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks, this is very cool, I'll change our implementation 🤝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧰 technical debts Technical debts, refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants