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

[RHCLOUD-18784] added function for closing test db connection #210

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

petracihalova
Copy link
Contributor

@petracihalova petracihalova commented Apr 8, 2022

RHCLOUD-18784

When I was working on tests in dao layer, I started to get this error:

2022/04/08 13:55:12 /Users/pcihalov/projects/sources-api-go/dao/main_test.go:53 
failed to connect to `host=localhost user=root database=sources_api_test_go`: server error 
(FATAL: sorry, too many clients already (SQLSTATE 53300))

When we call DoneWithFixtures(), the ConnectToTestDB("dao") is called again and again and connections are not closed. So I added function which closes this connection before we connect again to TestDB again.

@lpichler
Copy link
Contributor

lpichler commented Apr 8, 2022

Can we avoid closing connection as is pointed here ?

@petracihalova
Copy link
Contributor Author

Can we avoid closing connection as is pointed here ?

yes I know about this, but I chose this solution because I need it for my task I am working on now and it is easy fix for now ... but for sure it will be better to refactor this in future ... all tests in dao (dozens tests) use this logic and I believe refactor will be bigger change, but maybe I am wrong

@lpichler
Copy link
Contributor

lpichler commented Apr 8, 2022

Always I prefer don't add technical debt if it is possible - for now - maybe we can do just something with call ConnectToTestDB in DoneWithFixtures - it looks that we don't need to create new connection each time when we call DoneWithFixtures but we might create it once and then we can assign it back to DB in this function (as I read it in comment of DoneWithFixtures )

@MikelAlejoBR
Copy link
Member

I have run into the same issue in #208 . What I ended up doing was, as you did, closing the connection and reopening a new one for each time the tests are run, because I don't see any other way around Gorm. In fact I might have implemented that refactor that you linked, Petra.

Can we avoid closing connection as is pointed here ?

I tried it myself using some ridiculously high values for those parameters but it keeps failing, since it's Postgres the one that barks at this for having too many connections. So the quick-n-dirty fix would be to increase the maximum active connections in the database, but we would eventually face the same problem in the long run.

Always I prefer don't add technical debt if it is possible - for now - maybe we can do just something with call ConnectToTestDB in DoneWithFixtures - it looks that we don't need to create new connection each time when we call DoneWithFixtures but we might create it once and then we can assign it back to DB in this function (as I read it in comment of DoneWithFixtures )

The problem is that you can't modify Gorm's TablePrefix for an open connection, so if you create a new schema for your tests and try to migrate them, using SET search_path TO <schema> doesn't change Gorm's TablePrefix, and you end up migrating the schema the connection is pointing to.

The AutoMigrate doesn't allow you to change the schema, so I don't see any other way...

@petracihalova
Copy link
Contributor Author

This or change mentioned by Mikel in #208 is needed for #213

@petracihalova petracihalova changed the title added function for closing test db connection [RHCLOUD-18784] added function for closing test db connection Apr 11, 2022
@lindgrenj6 lindgrenj6 merged commit 66a5d64 into RedHatInsights:main Apr 11, 2022
@petracihalova petracihalova deleted the dao_tests_fix branch April 11, 2022 19:23
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.

None yet

4 participants