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

fix: use method from metadata for live component test helper #1434

Merged
merged 1 commit into from Feb 1, 2024

Conversation

daFish
Copy link
Contributor

@daFish daFish commented Jan 31, 2024

Q A
Bug fix? yes
New feature? no
Issues Fix #...
License MIT

After #1218 has been released as part of 2.14.0 my tests fail. This change passes the actual method to the requests made by the test helper.

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Jan 31, 2024
@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Jan 31, 2024
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

The change looks good, but I see that there are now failures inside the LiveComponent package itself.

@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Reviewed Has been reviewed by a maintainer labels Jan 31, 2024
@smnandre
Copy link
Collaborator

After #1218 has been released as part of 2.14.0 my tests fail. This change passes the actual method to the requests made by the test helper.

If you set "method: 'get'" on some of the tests that now fail, does it fix the problem or not ? (without the changes from this PR)

(just want to know if everything works in that case)

@daFish
Copy link
Contributor Author

daFish commented Jan 31, 2024

@smnandre Setting method: 'get' let's my test pass. Otherwise an Symfony\Component\HttpKernel\Exception\MethodNotAllowedHttpException is thrown.

@smnandre
Copy link
Collaborator

From your PR, could you try to, when in POST, send "props" in JSON and not in the query string ?

Someything "like that" (did not test at all)

  $this->client->request('POST', $this->router->generate(
            $this->metadata->get('route'),
            [
                '_live_component' => $this->metadata->getName(),
            ]
        ), [
            'json' => ['props' => $props->getProps()]
        ]);

@daFish
Copy link
Contributor Author

daFish commented Jan 31, 2024

This did the trick:

            'POST',
            $this->router->generate(
                $this->metadata->get('route'),
                [
                    '_live_component' => $this->metadata->getName(),
                ]
            ),
            [
                'data' => json_encode(['props' => $props->getProps()]),
            ],
        );

@smnandre
Copy link
Collaborator

@daFish good news! Could you update your PR to include this change, and we'll see then if tests are OK or if there is something else to fix :)

@daFish daFish force-pushed the fix/live-component-test-method branch from e3b4bf0 to 1d01162 Compare January 31, 2024 14:25
@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Jan 31, 2024
@daFish
Copy link
Contributor Author

daFish commented Jan 31, 2024

Looks like there is more work todo. I'll see if I can do something about it. Any hints are appreciated.

'props' => json_encode($props->getProps(), flags: \JSON_THROW_ON_ERROR),
]
));
$this->client->request($this->metadata->get('method'),
Copy link
Collaborator

@smnandre smnandre Jan 31, 2024

Choose a reason for hiding this comment

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

if ('POST' === $method = strtoupper($this->metadata->get('method'))) {
    $this->client->request(
        $method,
        $this->router->generate($this->metadata->get('route'), [
            '_live_component' => $this->metadata->getName(),
        ]),
        [
            'json' => ['props' => $props->getProps()],
        ],
    );
} else {
    $this->client->request($method, $this->router->generate(
        $this->metadata->get('route'),
        [
            '_live_component' => $this->metadata->getName(),
            'props' => json_encode($props->getProps(), flags: \JSON_THROW_ON_ERROR),
        ]
    ));
}

We need to handle both situations there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course - adjusted.

@@ -141,7 +144,7 @@ private function request(array $content = [], string $action = null): self
$csrfToken = $this->csrfToken();

$this->client->request(
'POST',
$this->metadata->get('method'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this one need any change... do your tests fail there ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

@daFish daFish force-pushed the fix/live-component-test-method branch from 1d01162 to 0498164 Compare January 31, 2024 14:56
@smnandre
Copy link
Collaborator

Do all your tests pass with this change? 😄

@daFish
Copy link
Contributor Author

daFish commented Jan 31, 2024

Yes, they do. And the checks here are also passing. But some more feedback from others would also be appreciated.

'_live_component' => $this->metadata->getName(),
]),
[
'data' => json_encode(['props' => $props->getProps()]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, we lost the flag :)

'data' => json_encode(['props' => $props->getProps()], flags: \JSON_THROW_ON_ERROR),

@daFish daFish force-pushed the fix/live-component-test-method branch from f0baa2a to d84f3eb Compare February 1, 2024 14:36
@daFish
Copy link
Contributor Author

daFish commented Feb 1, 2024

Failure unrelated.

@weaverryan
Copy link
Member

Thanks Marcus!

@weaverryan weaverryan merged commit 5a37a3e into symfony:2.x Feb 1, 2024
33 of 34 checks passed
@daFish daFish deleted the fix/live-component-test-method branch February 1, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants