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

Interacting with the database in the test teardown after calling the parent teardown is broken #49359

Closed
rikvdh opened this issue Dec 13, 2023 · 5 comments · Fixed by #49361
Closed
Labels

Comments

@rikvdh
Copy link
Contributor

rikvdh commented Dec 13, 2023

Laravel Version

10.37.1

PHP Version

8.2.11

Database Driver & Version

MySQL

Description

When overriding teardown to cleanup something in your tests the behavior is now broken in v10.37.1 due to the changes with #49327 , the database connection is gone.

imo behavioural changes like this are never allowed in bugfix-releases.

   │ Illuminate\Contracts\Container\BindingResolutionException: Target class [config] does not exist.
   │
   │ /var/www/vendor/laravel/framework/src/Illuminate/Container/Container.php:912
   │ /var/www/vendor/laravel/framework/src/Illuminate/Container/Container.php:795
   │ /var/www/vendor/laravel/framework/src/Illuminate/Foundation/Application.php:955
   │ /var/www/vendor/laravel/framework/src/Illuminate/Container/Container.php:731
   │ /var/www/vendor/laravel/framework/src/Illuminate/Foundation/Application.php:940
   │ /var/www/vendor/laravel/framework/src/Illuminate/Container/Container.php:1454
   │ /var/www/vendor/laravel/framework/src/Illuminate/Database/DatabaseManager.php:170
   │ /var/www/vendor/laravel/framework/src/Illuminate/Database/DatabaseManager.php:136
   │ /var/www/vendor/laravel/framework/src/Illuminate/Database/DatabaseManager.php:101
   │ /var/www/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php:1819
...

Steps To Reproduce

    public function tearDown(): void
    {
        parent::tearDown();
        $this->model->delete();
    }
@driesvints
Copy link
Member

Sorry about that. We'll revert it: #49361

@mpyw
Copy link
Contributor

mpyw commented Dec 14, 2023

@rikvdh I think this is expected behavior:

  • parent::setUp() must be called first
    public function setUp(): void
    {
        parent::setUp();
        // Your Code Goes Here
    }
  • parent::tearDown() must be called later
    public function tearDown(): void
    {
        // Your Code Goes Here
        parent::tearDown();
    }

While this was a significant change, your original code has a major problem.

@driesvints @taylorotwell Why did you closed #49362?

@mpyw
Copy link
Contributor

mpyw commented Dec 14, 2023

It is odd that model deletion works after parent::tearDown(), since facades are deactivated after parent::tearDown().

  • This doesn't work:
    public function tearDown(): void
    {
        parent::tearDown();
        DB::table($this->model->getTable())->where('id', $this->model->id)->delete();
    }
  • UNEXPECEDLY this works:
    public function tearDown(): void
    {
        parent::tearDown();
        $this->model->delete();
    }

@rikvdh
Copy link
Contributor Author

rikvdh commented Dec 14, 2023

@mpyw

.. your original code has a major problem.

It might not be, by definition, my code, if you have 100's of testcases with 1000+ tests written by 10+ developers over the past 10 years things like this slip in. "My code" has 100's of 'major problems' every day, and fixing up 10 year old tests because behavioral changes in a bugfix release is no fun.

@mpyw
Copy link
Contributor

mpyw commented Dec 14, 2023

@rikvdh It is hard to deal with code that is several years old, and I understand your difficulty. I do feel you are correct to the point where it was reverted once by this report, as it was certainly unexpected as a fix to 10.x. However, I felt it was odd that the resubmission for 11.x was reverted, so I raised a discussion. Thank you for your cooperation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants