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

Failing tests #588

Closed
martin-ro opened this issue Oct 14, 2022 · 6 comments · Fixed by #589
Closed

Failing tests #588

martin-ro opened this issue Oct 14, 2022 · 6 comments · Fixed by #589
Assignees
Labels
question Further information is requested

Comments

@martin-ro
Copy link

Describe your task
When running my tests I get Working inside an embedded transaction is not possible.

To Reproduce
Steps to reproduce the behavior:
Create a test:

<?php

use App\Models\User;
use function Pest\Laravel\actingAs;
use function Pest\Laravel\get;

uses()->group('WalletTest');

test('fails', function () {
    $user = User::factory()->asStudent()->create();

    actingAs($user)
        ->get('/')
        ->assertOk();
});

test('works', function () {
    $user = User::factory()->asStudent()->create();

    get('/')->assertOk();
});

Trace Error

 Working inside an embedded transaction is not possible. https://bavix.github.io/laravel-wallet/#/transaction

  at tests/Feature/WallteTest.php:14
     10▕     $user = User::factory()->asStudent()->create();
     11▕ 
     12▕     actingAs($user)
     13▕         ->get('/')
  ➜  14▕         ->assertOk();
     15▕ });
     16▕ 
     17▕ test('works', function () {
     18▕     $user = User::factory()->asStudent()->create();



  Tests:  1 failed, 1 passed
  Time:   4.52s

Expected behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Server:

  • php version: 8.1
  • database: mysql 8.0
  • wallet version 9.5.0
  • cache lock: redis
  • cache wallets: redis

Additional context
I've read #463 but this behavior still doesn't really make any sense to me. What DB transaction is not closed using actingAs?

@martin-ro martin-ro added the question Further information is requested label Oct 14, 2022
@rez1dent3
Copy link
Member

Hello, @martin-ro. Inside the package for performance, a kind of saga is implemented. The work is described here #412 and here #564. Tests (pest) start with a transaction, which prevents correct verification. The package itself controls the race condition, locks and rolls back the state in case of an error in the transaction. Unfortunately, there are no other solutions available at the moment. This is a technical limitation that had to be taken to improve fault tolerance and performance.

@martin-ro
Copy link
Author

Hey @rez1dent3, thanks for taking the time to explain. I figured out that in my case tests are failing because I display a wallet balance in the front end.

Currently, that's the only use case where I have trouble. Is there a way to get to the wallet balance without triggering an atomic lock? I think it would be helpful if the package offered some convenient method to do some operations without locks. Something like $user->wallet->withoutBlock()->balance or $user->wallet->copy()->balance.

Do you think there is some way to get this to work?

It's really a significant problem, the wallet obviously needs atomic locks but also I can't use it if I can't run the test suite anymore 😅

@rez1dent3
Copy link
Member

@martin-ro If you are interested in the balance within the tests, then get directly from the table. This can be done in the following way:

$wallet->getOriginalBalanceAttribute();

You can try to write your own implementation of LockServiceInterface and change it through the config, but most likely you will have to change the DatabaseServiceInterface as well.


I have ideas how I can drop my implementation of the DatabaseServiceInterface, but it is entirely framework dependent. I need a "committing" event for the commit method inside ManagesTransactions. Without this change, I will have to leave the current solution. Here is a link to the PR in the framework: laravel/framework#44608

The class can also be redefined inside the package, but there is a high probability of not keeping track of the changes. And this will negatively affect the support of the package.

@martin-ro
Copy link
Author

In my case I got all tests working with $wallet->getOriginalBalanceAttribute();. Thanks!

This was linked to pull requests Oct 19, 2022
@rez1dent3 rez1dent3 removed a link to a pull request Oct 19, 2022
@rez1dent3
Copy link
Member

Hello, @martin-ro. Tag 9.6.0.
In theory, the problem should go away

@martin-ro
Copy link
Author

Thanks a lot!

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

Successfully merging a pull request may close this issue.

2 participants