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

Refactor the tests for newer Phpunit version #201

Closed
wants to merge 1 commit into from

Conversation

Zvax
Copy link
Contributor

@Zvax Zvax commented Nov 25, 2024

After the nullable typehint PR, there's been a few commits to bump the PHP version, normalize test namespace and move towards github actions.

This PR attempts to update the testsuite to best accommodate these changes, though I have made some judgement calls:

  1. I've bumped the phpunit version to >= 7, such that php versions will be able to resolve the composer file. using ^7 or ^8 would fail either on 8.x or <=7.1
  2. I've replaced the fabpot/php-cs-fixer with the recommended package visible in the composer commands, that is friendsofphp/php-cs-fixer. I've renamed the config file as advised, though unsure what should be the new config, and there doesn't seem to be evident configs in the ecample file https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/master/.php-cs-fixer.dist.php
  3. Change the 'Auryn\Type' strings to Type::class invocations where pertinent
  4. refactor the deprecated @expectException comments to $this->expectException and related calls
  5. Add some checks on the phpversion to account for the change of behaviour of the Class not found php exception message (from php > 8 class is surrounded by double wuotes)
  6. added explicit properties where implicit property declaration was tripping tests
  7. slapped a bunch of assertTrue(true) where tests were saying having no assertions. I presume if the rest of the tests didn't throw then we consider it valid.
  8. add return types to the tests

I've went as far as I could with solving the tests, however on 8.x suites there are a few failures that necessitate a better knowedge of what should happen than I possess (please see https://github.com/Zvax/auryn/actions/runs/12017589097). I think they are related, it seems like the nullable instances with default values aren't being passed, or something.

There were 4 failures:

1) Auryn\InjectorTest::testNonConcreteDependencyWithDefaultValueThroughAlias
Failed asserting that null is an instance of class "Auryn\ImplementsInterface".

/home/runner/work/auryn/auryn/test/InjectorTest.php:765

2) Auryn\InjectorTest::testNonConcreteDependencyWithDefaultValueThroughDelegation
Failed asserting that null is an instance of class "Auryn\ImplementsInterface".

/home/runner/work/auryn/auryn/test/InjectorTest.php:774

3) Auryn\InjectorTest::testDependencyWithDefaultValueThroughShare
Failed asserting that null is an instance of class "StdClass".

/home/runner/work/auryn/auryn/test/InjectorTest.php:788

4) Auryn\InjectorTest::testDelegationDoesntMakeObject
Failed asserting that exception of type "TypeError" matches expected exception "Auryn\InjectionException". Message was: "class_implements(): Argument #1 ($object_or_class) must be of type object|string, null given" at
/home/runner/work/auryn/auryn/lib/Injector.php:596
/home/runner/work/auryn/auryn/lib/Injector.php:376
/home/runner/work/auryn/auryn/test/InjectorTest.php:1123

As far as I can teel this seems like the behaviour actually changes so I don't know what should be changed.

Please let me know if/how these should be fixed.

@Zvax Zvax force-pushed the bugfix/namespace branch 6 times, most recently from 8b9e88f to 215d2de Compare November 26, 2024 14:34
@Zvax Zvax closed this Nov 26, 2024
@kelunik
Copy link
Collaborator

kelunik commented Nov 26, 2024

Thanks! Why did you close this?

@Zvax
Copy link
Contributor Author

Zvax commented Nov 26, 2024

after I realized I was spamming everyone with my test runs, I wanted to close it until I could make the runs pass. Please see the updated passing PR!

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.

2 participants