pestphp / pest-plugin-drift

The Pest Drift Plugin
https://pestphp.com
MIT License
68 stars 7 forks source link

Fix assertJson Method Conflict in PHPUnit to Pest Conversion for Laravel Project #26

Closed alighorbani1381 closed 1 year ago

alighorbani1381 commented 1 year ago

Hey :wave:,

First and foremost, I'd like to express my appreciation to the Pest team for developing the drift plugin, which has been invaluable for transitioning a large number of PHPUnit tests to this fantastic wrapper :rose:.

However, I've encountered a bug with the plugin, specifically in scenarios where I've used the drift plugin to convert legacy PHPUnit tests into Pest syntax in Laravel projects. Unfortunately, this has resulted in several test failures :fearful:.

Let's Dive into the Issue with Some Code

<?php

use Illuminate\Foundation\Testing\TestCase;

class UserCreationTest extends TestCase
{
    public function test_can_store_user_data()
    {
        $data = ["name" => "ali"];

        $response = $this->postJson(route("users.store"), $data);

        $response->assertCreated();

        $response->assertJson([
            "data" => [
                "name" => $data["name"]
            ]
        ]);
    }
}

Our Expected Outcome ✅

We anticipate that after converting this test into the code below:

<?php

test('can store user data', function () {
    $data = ["name" => "ali"];

    $response = $this->postJson(route("users.store"), $data);

    $response->assertCreated();

    expect($response)->assertJson([
        "data" => [
            "name" => $data["name"]
        ]
    ]);
});

But the Actual Pest Conversion Result is ❌

Regrettably, the generated Pest code is not as expected:

<?php

test('can store user data', function () {
    $data = ["name" => "ali"];

    $response = $this->postJson(route("users.store"), $data);

    $response->assertCreated();

    expect([
        "data" => [
            "name" => $data["name"]
        ]
    ])->toBeJson();  // 👈️ This is incorrect!
});

Why Did This Issue Occur?

This issue arises because Pest has introduced some assertions with names identical to those in both Laravel and PHPUnit, causing conflicts. This leads to unexpected behavior when transitioning to Pest syntax (Expectations), as demonstrated by the example above.

(PR) Solution!

I'm thrilled to inform you that I've identified a solution to resolve the problem present in this merge request. Please take a moment to review the proposed changes and provide your feedback. Let's collaborate to ensure that our tests continue to run smoothly and our codebase remains robust.

Thanks for your attention and support! :raised_hands:

mandisma commented 1 year ago

Hey :slightly_smiling_face:

Thank you for your work and your proposition.

I was just thinking that your modifications could be made globally to handle future issues like this. Maybe update the AbstractAssertionToExpectation and check if the assertion is called with $this or not could be enough.

alighorbani1381 commented 1 year ago

Hey :slightly_smiling_face:

Thank you for your work and your proposition.

I was just thinking that your modifications could be made globally to handle future issues like this. Maybe update the AbstractAssertionToExpectation and check if the assertion is called with $this or not could be enough.

Okay and what is your solution to handle that?

GeniJaho commented 1 year ago

I want to help with this issue, can I submit a PR to the branch? Right now the Drift plugin is unusable because of this bug.

mandisma commented 1 year ago

Hi @alighorbani1381

Thank you for your pull request and your efforts to improve our project.

After a careful review of your changes, we've identified that the proposed solution is quite specific and may not cover the problem globally. While your approach has merit in certain scenarios, we had implemented a solution that addresses the issue on a broader scale.

Thank you once again for your contribution, and we look forward to your future involvement in our project.

Best regards