Skip to content
This repository has been archived by the owner on May 11, 2021. It is now read-only.

[fix] destroy connection on test teardown #281

Merged
merged 3 commits into from
Dec 6, 2020

Conversation

georgehristov
Copy link
Collaborator

@georgehristov georgehristov commented Dec 4, 2020

Fix for Burn test "Too many connections" failure

Copy link
Member

@DarkSide666 DarkSide666 left a comment

Choose a reason for hiding this comment

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

LGTM.
Tests fail because new PHPUnit 9.5 doesn't have getAnnotations() method anymore.

Copy link
Member

@mvorisek mvorisek left a comment

Choose a reason for hiding this comment

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

in Oracle, we have a special PDO reuse, maybe for this reason, try to remove it

if the tests pass then, then this is probably welcomed

also, what about the another test cases?

@georgehristov
Copy link
Collaborator Author

@mvorisek I saw tests failing (on MySQL) due to too many connections and I applied same fix as in data.
Just to describe it in words: the issue is that if a model is stored in test class property on test setUp then the connection is not closed on test tearDown unless this property is unset.
So each test creates one more connection until the connection limit is reached. If number of tests is below that limit then all appears fine.

@mvorisek
Copy link
Member

mvorisek commented Dec 6, 2020

I am not arguing it, see my additional commit about that Oracle issue. If these fixed by you solves it now here and in data, than both 👍

@mvorisek
Copy link
Member

mvorisek commented Dec 6, 2020

Oracle is not fixed by this... I think this fix is "correct" in sense of cleaness, ie. what is setup is released too.

@mvorisek mvorisek merged commit b1dc6c8 into develop Dec 6, 2020
@mvorisek mvorisek deleted the fix/destroy-test-connection branch December 6, 2020 11:56
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.

3 participants