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 for issue #6519 (Pretty URLs in tests) #6661

Merged

Conversation

FoxCie
Copy link

@FoxCie FoxCie commented Dec 20, 2024

When using pretty URLs in tests, the AdminUrlGenerator fails to provide the pretty URLs because it checks the use of pretty URLs through the context, which is null when doing requests from tests. This fixes this behaviour by checking the use of pretty URLs directly from the UrlGenerator (Router), which is what is done by the context anyway.

Tests have been adapted to use pretty URLs.

@javiereguiluz
Copy link
Collaborator

@FoxCie I'm not sure about these proposed changes.

Currently, we have separate tests for ugly URLs and pretty URLs. See

- name: Run tests
continue-on-error: ${{ matrix.stability == 'dev' || matrix.symfony_version == '7.2' }}
env:
SYMFONY_DEPRECATIONS_HELPER: "max[total]=999999&ignoreFile=./tests/baseline-ignore.txt"
run: |
./vendor/bin/simple-phpunit --exclude-group pretty_urls
- name: Run Pretty URLs tests
# Symfony 5.4 version doesn't include the #[Route] attribute needed to run these tests (it only includes the @Route annotation)
if: ${{ matrix.symfony_version != '5.4' && matrix.composer_args != '--prefer-lowest' }}
continue-on-error: ${{ matrix.stability == 'dev' || matrix.symfony_version == '7.2' }}
env:
USE_PRETTY_URLS: "1"
SYMFONY_DEPRECATIONS_HELPER: "max[total]=999999&ignoreFile=./tests/baseline-ignore.txt"
run: |
./vendor/bin/simple-phpunit tests/Controller/PrettyUrls/PrettyUrlsControllerTest.php

I prefer to have those two test suites separate because mixing pretty URLs with the existing tests that used the traditional ugly URLs is a mess in a my opinion. I'd prefer to keep both separated and then updated all tests to use pretty URLs only in the 5.x branch, which I hope to start soon.

@FoxCie
Copy link
Author

FoxCie commented Dec 28, 2024

Hello @javiereguiluz

I reverted the changes to the tests, and added one in the PrettyUrlControllerTest to show that the AdminUrlGenerator does not behave well in tests when using pretty urls prior to this patch. This is not related to any controller, but since tests are invoked with this class only I added it there.

@javiereguiluz javiereguiluz added this to the 4.x milestone Jan 2, 2025
@javiereguiluz javiereguiluz force-pushed the foxcie/issue_6519_pretty_urls_in_tests branch from 40e16f6 to 8fdbca1 Compare January 2, 2025 18:49
@javiereguiluz
Copy link
Collaborator

Thanks a lot for fixing this bug, providing new tests ... and for your patience and the extra effort during the review process. This is merged now!

@javiereguiluz javiereguiluz merged commit 0a025ee into EasyCorp:4.x Jan 2, 2025
12 checks passed
@FoxCie FoxCie deleted the foxcie/issue_6519_pretty_urls_in_tests branch January 6, 2025 09:40
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 this pull request may close these issues.

2 participants