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

Make tests compatible with newer phpunit versions #7727

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

williamdes
Copy link

Description:

  • Non breaking changes for newer phpunit versions (running on PHPUnit 11.1.3 at Debian)
  • Fixing the parameter names for newer PHP versions (Error: Unknown named parameter $check)

Review

@williamdes
Copy link
Author

Variable "user_agent" is not in valid camel caps format

Can someone give me the best way to fix this. Not by changing the variable name for sure 😄

@sgiehl
Copy link
Member

sgiehl commented Jul 2, 2024

Why should the variable name need to contain a _? What is the problem with useragent? This doesn't comply with our coding style.

@williamdes
Copy link
Author

Why should the variable name need to contain a _? What is the problem with useragent? This doesn't comply with our coding style.

Nope, because as you can see getTypeMethodFixtures is directly fed from \Spyc::YAMLLoad($fixturePath);
So the variable names should match the yaml data key names

One solution is that I can put an array map to re-format the key name

@williamdes

This comment was marked as resolved.

@williamdes williamdes marked this pull request as draft July 17, 2024 10:41
@williamdes williamdes force-pushed the phpunit-upgrade branch 3 times, most recently from 76b7e67 to 7298a44 Compare July 17, 2024 11:19
@williamdes williamdes marked this pull request as ready for review July 17, 2024 11:19
@williamdes williamdes requested a review from sanchezzzhak July 17, 2024 11:20
@williamdes
Copy link
Author

williamdes commented Jul 17, 2024

Weird CI failures, new commits broke something or is it my diff ?

@liviuconcioiu
Copy link
Collaborator

liviuconcioiu commented Jul 17, 2024

Not sure, but probably the PHPUnit workflow should be updated too so it can use composer update? Now it tries to run PHPUnit 11 on PHP 7.2.

I installed PHP 7.2.5 (this is the lowest version required by symfony/yaml 5.1.7) and the tests pass.

@williamdes
Copy link
Author

Not sure, but probably the PHPUnit workflow should be updated too so it can use composer update? Now it tries to run PHPUnit 11 on PHP 7.2.

I installed PHP 7.2.5 (this is the lowest version required by symfony/yaml 5.1.7) and the tests pass.

Oh snap, let's revert that part. I only need the PHP diff anyway
PRs like #7570 can update that

.gitignore Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@williamdes
Copy link
Author

I dropped the last commit, it should help

@liviuconcioiu
Copy link
Collaborator

@williamdes can you fix the conflicts? And also you should also add static to getFixturesDeviceTypeFromClientHints. It was added after your PR.

@williamdes
Copy link
Author

I did some ajustements as I am quite sure the ignore should not have been used for all versions and this way.

@sanchezzzhak
Copy link
Collaborator

Run local php vendor/bin/phpcbf for fix phpcs

@williamdes
Copy link
Author

williamdes commented Dec 24, 2024

Run local php vendor/bin/phpcbf for fix phpcs

Unrelated, see CI log

PHP Parse error: syntax error, unexpected '|', expecting variable (T_VARIABLE) in /home/runner/work/device-detector/device-detector/vendor/phpunit/phpunit/src/Framework/Assert/Functions.php on line 83

Probably some issue with the php version installed

Edit: pushed a fix, maybe it will work

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

Successfully merging this pull request may close these issues.

4 participants