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

Tests fail when database is used at application boostrap #98

Open
warkadiuszz opened this issue Feb 3, 2024 · 2 comments
Open

Tests fail when database is used at application boostrap #98

warkadiuszz opened this issue Feb 3, 2024 · 2 comments

Comments

@warkadiuszz
Copy link
Contributor

Hello,

after adding a function to the boostrap of my Yii2 application, a lot of my tests stopped working. For all of them it was always one of two reasons:

  1. TooManyConnections (just as the other old issues that happened in the past in that module, eg. here and here).
  2. Data in the database not being cleared after each test (similar to this)

All tests started working again immediately after removing the function in boostrap. Imporantly, the additional code in the boostrap uses database. For example (config main.php):

'bootstrap' => [
  'something' => function () {
    Model::find()->all();
  }
],

Explanation / debugging / working fix

What I was able to find out is that both ConnectionWatcher and TransactionForcer classes work by hooking to Connection::EVENT_AFTER_OPEN event. When some code in boostrap uses database, before those two watchers are set up, the EVENT_AFTER_OPEN happens before them, so they never trigger.

The order of the code in Module/Yii2::_before (here) is:

1: $this->client->startApp($this->yiiLogger);
 
2: $this->connectionWatcher = new Yii2Connector\ConnectionWatcher();
3: $this->connectionWatcher->start();

// ...

4: $this->startTransactions();

Application boostrap happens at step (1) - startApp. If database is used at that point, Connection::EVENT_AFTER_OPEN also happens. Then the handlers created by (2), (3) and (4) never work again(?), because they are waiting for that event to happen.

If instead the startApp is called last:

2: $this->connectionWatcher = new Yii2Connector\ConnectionWatcher();
3: $this->connectionWatcher->start();

// ...

4: $this->startTransactions();
1: $this->client->startApp($this->yiiLogger);

Then both ConnectionWatcher and TransactionForcer (startTransactions()) bind to Connection::EVENT_AFTER_OPEN before bootstrap so they work correctly.


Steps to reproduce

  1. In application config add function that calls DB:
'bootstrap' => [
  'something' => function () {
    SomeModel::find()->all();
  }
],
  1. Create a test with 2 cases that relies on data being cleaned between them:
<?php

namespace app\tests\unit;

use Codeception\Test\Unit;


class SomeTest extends Unit
{
    public function testCaseOne(): void
    {
	// Ensure no records are present in DB        
	$this->assertEquals(0, SomeModel::find()->count());
	
	// Create record
	$m = new SomeModel();
	$m->save();


	$this->assertEquals(1, SomeModel::find()->count());
    }

    public function testCaseTwo(): void
    {
	// Ensure no records are present in DB
	// This will fail if boostrap function using database is present
	$this->assertEquals(0, SomeModel::find()->count());
    }
}

With all that being said, I'm not sure if using database during application bootstrap is even legal. Offical Yii2 documentation does not seem to prohibit that though, at least I was not able to find such information. Also everything works.
Another thing is – I'm not sure if the order of those functions calls can be changed at all and if it won't be a breaking change for anyone else. It certainly works for me and, in my case, it has been broken since this rework from years ago. For now I keep that change in private fork but if the maintainers would be interested, I'll be happy to open a PR.

@samdark
Copy link
Collaborator

samdark commented Feb 10, 2024

I'm not sure if using database during application bootstrap is even legal

As you can see, that usage is tricky.

Another thing is – I'm not sure if the order of those functions calls can be changed at all and if it won't be a breaking change for anyone else.

Likely it would break something. @SamMousa since you did that rework, do you remember order change reasoning?

@SamMousa
Copy link
Collaborator

I don't see an issue directly with changing the order. When do we remove class level event listeners? If we don't do that before starting the app in startApp it should be fine

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

No branches or pull requests

3 participants