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

Remove jest testcontainers #13350

Merged
merged 19 commits into from Mar 26, 2024
Merged

Remove jest testcontainers #13350

merged 19 commits into from Mar 26, 2024

Conversation

samwho
Copy link
Collaborator

@samwho samwho commented Mar 25, 2024

Description

While working on the Postgres unmocking PR, I noticed that when we run tests, there are two copies of the testcontainers/ryuk container running. One as version 0.3.0 and another as 0.5.1.

On closer inspection, it appears that the jest-testcontainers package is depending on testcontainers@4.7.0 while the Budibase project depends on testcontainers@10.7.2.

This discrepancy doesn't seem ideal, so I've removed jest-testcontainers in the hope it may help with some of the flakiness we've been seeing. I have no proof that it will, but this at least removes this discrepancy as a culprit.

Pro PR: https://github.com/Budibase/budibase-pro/pull/285

@@ -27,7 +27,7 @@ class Replication {
return resolve(info)
})
.on("error", function (err) {
throw new Error(`Replication Error: ${err}`)
throw err
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This caused an error in a test while I was working on this change, and I moved to throwing the raw error in order to see the full stack trace. Decided to leave it as is because I feel this is more useful.

@@ -1,85 +1,66 @@
import { DatabaseImpl } from "../../../src/db"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've changed how this file works because when I moved away from jest-testcontainers, this code started crafting invalid CouchDB URLs.

The new code uses docker ps --format json to make parsing the output easier, and it also makes sure to always return testcontainers containers by looking for the org.testcontainers=true label.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a nice improvement, I didn't even know you could get JSON out here, very handy!

@@ -0,0 +1,2 @@
[log]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've taken the opportunity here to calm the CouchDB logs down. By default they log every single HTTP request, which is an unbelievable amount of logging. This made DEBUG=testcontainers* unusable, whereas with level = warn the output is a little less huge and more useful.

@samwho samwho marked this pull request as ready for review March 25, 2024 17:49
@samwho samwho requested a review from a team as a code owner March 25, 2024 17:49
@samwho samwho requested review from adrinr and removed request for a team March 25, 2024 17:49
packages/backend-core/src/tests/globalTeardown.ts Outdated Show resolved Hide resolved
packages/server/src/tests/globalSetup.ts Outdated Show resolved Hide resolved
packages/server/src/tests/globalSetup.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@mike12345567 mike12345567 left a comment

Choose a reason for hiding this comment

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

This is a nice improvement, using testcontainers more directly gives us a lot more control and removes a potential candidate for the flakiness we've been seeing, great work!

@samwho samwho enabled auto-merge March 26, 2024 10:12
@samwho samwho disabled auto-merge March 26, 2024 10:12
Copy link
Collaborator

@adrinr adrinr left a comment

Choose a reason for hiding this comment

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

Nice to see some much love given to the tests :D

@samwho samwho enabled auto-merge March 26, 2024 10:17
@samwho samwho merged commit df52b69 into master Mar 26, 2024
10 of 11 checks passed
@samwho samwho deleted the remove-jest-testcontainers branch March 26, 2024 10:25
@github-actions github-actions bot locked and limited conversation to collaborators Mar 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants