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

mock postgres database v1 in more tests [SW-403] #2064

Merged
merged 1 commit into from
Oct 28, 2024
Merged

Conversation

PooyaRaki
Copy link
Contributor

@PooyaRaki PooyaRaki commented Oct 27, 2024

Summary

Added PostgresDatabaseModule mock to additional unit tests to enhance test coverage and reliability.
When registering Notification V2, we encountered a "too many connections" error. To address this, we need to mock Database V1 and V2 in all tests. This PR adds the missing mocks.

Changes

  • Updated existing unit tests to utilize the mock for improved isolation and consistency.
  • Verified that tests maintain expected behavior with the mock implementation.

How to test/reproduce

@PooyaRaki PooyaRaki self-assigned this Oct 27, 2024
@PooyaRaki PooyaRaki marked this pull request as ready for review October 27, 2024 16:54
@PooyaRaki PooyaRaki requested a review from a team as a code owner October 27, 2024 16:54
@PooyaRaki PooyaRaki changed the title mock postgres database v1 in more tests mock postgres database v1 in more tests [SW-403] Oct 27, 2024
@hectorgomezv
Copy link
Member

Check out branch https://github.com/safe-global/safe-client-gateway/pull/2063.
Run the unit tests.
You should encounter the "too many connections" error.

I think the error there is related to a non-graceful app shutdown during the test execution, so maybe the database connections are not properly closed in your local setup. You can check the error in the CI there is

TypeError: Configuration key "jwt.issuer" does not exist

If the dependency graph is modified on purpose (and the auth-related modules are needed at startup), then you need to include placeholders for JWT_ISSUER and JWT_SECRET.

@PooyaRaki
Copy link
Contributor Author

Check out branch https://github.com/safe-global/safe-client-gateway/pull/2063.
Run the unit tests.
You should encounter the "too many connections" error.

I think the error there is related to a non-graceful app shutdown during the test execution, so maybe the database connections are not properly closed in your local setup. You can check the error in the CI there is

TypeError: Configuration key "jwt.issuer" does not exist

If the dependency graph is modified on purpose (and the auth-related modules are needed at startup), then you need to include placeholders for JWT_ISSUER and JWT_SECRET.

I believe that’s a different issue. This PR addresses a problem with the unit tests, whereas the issue you’re seeing in the PR is related to the e2e tests. You need to run unit tests and monitor your database connections as well to face the issue this PR fixes.

@hectorgomezv
Copy link
Member

I believe that’s a different issue. This PR addresses a problem with the unit tests, whereas the issue you’re seeing in the PR is related to the e2e tests. You need to run unit tests and monitor your database connections as well to face the issue this PR fixes.

The issue in the other branch is related to the unit tests, not e2e, as you can see in the screenshot.

Screenshot 2024-10-28 at 14 40 46

But I'm fine with the changes in the PR 👍 Thanks for looking into this @PooyaRaki !

@PooyaRaki
Copy link
Contributor Author

I believe that’s a different issue. This PR addresses a problem with the unit tests, whereas the issue you’re seeing in the PR is related to the e2e tests. You need to run unit tests and monitor your database connections as well to face the issue this PR fixes.

The issue in the other branch is related to the unit tests, not e2e, as you can see in the screenshot.

Screenshot 2024-10-28 at 14 40 46

But I'm fine with the changes in the PR 👍 Thanks for looking into this @PooyaRaki !

Initially, it was also the e2e tests that were affected. Please take a look:
https://github.com/safe-global/safe-client-gateway/actions/runs/11542383150

@PooyaRaki PooyaRaki merged commit aa9c587 into main Oct 28, 2024
20 checks passed
@PooyaRaki PooyaRaki deleted the postgresMock branch October 28, 2024 13:50
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.

3 participants