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 tests #1065

Merged
merged 5 commits into from
Sep 2, 2024
Merged

Fix tests #1065

merged 5 commits into from
Sep 2, 2024

Conversation

VincentLanglet
Copy link
Contributor

I think there is a change in the behavior of some dependency which remove this useless parenthesis when parsing the file.

($file instanceof \SplFileInfo) ? $file : new \SplFileInfo($file);

is transformed to

$file instanceof \SplFileInfo ? $file : new \SplFileInfo($file);

Since the parenthesis is useless, it seems better to remove it from the both input and output IMHO.

@VincentLanglet
Copy link
Contributor Author

I don't understand the failure https://github.com/humbug/php-scoper/actions/runs/10401466964/job/28820621264?pr=1065 ; it might be something random.

For the removal of the parenthesis it comes from the next version of nikic/php-parser
https://github.com/nikic/PHP-Parser/releases/tag/v5.1.0

So I had to update the version in the composer.lock in order to use the same for all the tests @theofidry

@VincentLanglet
Copy link
Contributor Author

Can you approve again the CI @theofidry, to see if the PR is ok or need more fixes ?

Copy link
Member

@theofidry theofidry left a comment

Choose a reason for hiding this comment

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

the unit test failures looks related to some changes in the jetbrains stubs. I don't know if that was intentional of them or not, and it's worth noting that those stubs are about the IDE, they are not meant to provide an exhaustive list of symbols (e.g. there's plenty of old unmaintained extensions that are not documented).

In any case such changes go to the Enriched reflector. Usually I try to keep the reflector as light as possible, but add more tests for checking against regressions. If it's not obvious I may be able to take a lookt this week

@@ -31,7 +31,7 @@ class GAE2ETest extends TestCase

public function test_github_actions_executes_all_the_e2e_tests(): void
{
$expected = array_diff(E2ECollector::getE2ENames(), self::IGNORED_E2E_TESTS);
$expected = array_values(array_diff(E2ECollector::getE2ENames(), self::IGNORED_E2E_TESTS));
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change: isn't the assertEqualsCanonicalizing() supposed to not care about the keys difference?

@VincentLanglet
Copy link
Contributor Author

the unit test failures looks related to some changes in the jetbrains stubs. I don't know if that was intentional of them or not, and it's worth noting that those stubs are about the IDE, they are not meant to provide an exhaustive list of symbols (e.g. there's plenty of old unmaintained extensions that are not documented).

In any case such changes go to the Enriched reflector. Usually I try to keep the reflector as light as possible, but add more tests for checking against regressions. If it's not obvious I may be able to take a lookt this week

ares_gethostbyname was removed in JetBrains/phpstorm-stubs#1627 because it's considered as outdated.

I assume it's the same for
uv_read2_start
uv_handle_type
uv_ares_init_options

Do you know these method ? Is the PR on phpstorm-stubs wrong ?

@theofidry
Copy link
Member

I don't know them, but it's what I meant by "those stubs are about the IDE, they are not meant to provide an exhaustive list of symbols" when talking of JetBrains stubs. So whilst we can use it to leverage a big number of stubs, that list needs to be complemented.

The missing symbols can be added to https://github.com/humbug/php-scoper/blob/main/src/Symbol/Reflector.php

@VincentLanglet
Copy link
Contributor Author

The missing symbols can be added to main/src/Symbol/Reflector.php

Done @theofidry

@theofidry theofidry merged commit 1cfc944 into humbug:main Sep 2, 2024
121 of 126 checks passed
@theofidry
Copy link
Member

Thank you @VincentLanglet!

@VincentLanglet
Copy link
Contributor Author

Thank you @VincentLanglet!

Thanks you too.
I rebased #1061

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