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

Fixed #5230: @runClassInSeparateProcess has the same effect as @runTestsInSeparateProcesses #5233

Open
wants to merge 1 commit into
base: 10.5
Choose a base branch
from

Conversation

WalterWoshid
Copy link

@WalterWoshid WalterWoshid commented Feb 22, 2023

Did some refactoring and now it works :)

I also did some testing for my personal use case and it works!!!

@WalterWoshid WalterWoshid changed the title Fixed #5230 Fixed #5230: @runClassInSeparateProcess has the same effect as @runTestsInSeparateProcesses Feb 22, 2023
@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (a6319b7) 89.80% compared to head (bdf2488) 81.34%.

❗ Current head bdf2488 differs from pull request most recent head 546f158. Consider uploading reports for the commit 546f158 to get more accurate results

Files Patch % Lines
src/Util/PHP/AbstractPhpProcess.php 79.31% 6 Missing ⚠️
src/Framework/TestSuite.php 94.73% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               10.5    #5233      +/-   ##
============================================
- Coverage     89.80%   81.34%   -8.47%     
+ Complexity     6420     5866     -554     
============================================
  Files           680      629      -51     
  Lines         20389    18564    -1825     
============================================
- Hits          18311    15100    -3211     
- Misses         2078     3464    +1386     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@WalterWoshid
Copy link
Author

@ob_end_clean(); in the template sometimes logs failed to delete buffer. No buffer to delete to the console. Not sure what it's needed for, maybe someone can look into it and place it at the right position

@sebastianbergmann
Copy link
Owner

I am sorry that I was not able to review this pull request sooner. Can you please explain on a conceptual what was wrong and how you fixed it? Thanks!

This pull request has conflicts that must be resolved. It also targets the main branch. It should target the 10.5 branch now (it should have targetted the 10.1 branch when it was created).

@WalterWoshid
Copy link
Author

Hi @sebastianbergmann,

as far as I remember, the logic for @runClassInSeparateProcess and @runTestsInSeparateProcesses was placed in the same loop for running tests which made it actually impossible to run the class in a separate process, which is why I moved the @runClassInSeparateProcess logic from TestCase to TestSuite. If you'd like, I can resolve the merge conflicts :)

@sebastianbergmann
Copy link
Owner

If you'd like, I can resolve the merge conflicts :)

Yes, please. Thanks!

@WalterWoshid
Copy link
Author

Done :)

@WalterWoshid
Copy link
Author

Sorry for the multiple actions, forgot the guidelines, then the type checking.

@sebastianbergmann
Copy link
Owner

Sorry for the multiple actions, forgot the guidelines, then the type checking.

No worries, I don't mind clicking that button ;)

@WalterWoshid
Copy link
Author

No worries, I don't mind clicking that button ;)

Good, because I changed the wrong variable and didn't check the tests again x(

Should be good now 👌

@sebastianbergmann
Copy link
Owner

Can you have a look into why the tests fail on PHP 8.3 and PHP 8.4? Thanks!

@@ -16,7 +16,7 @@ if (!defined('STDOUT')) {

{iniSettings}
ini_set('display_errors', 'stderr');
set_include_path('{include_path}');
set_include_path(unserialize('{include_path}'));

Choose a reason for hiding this comment

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

Why are serialize() / unserialize() now used for the include_path?

@@ -50,7 +50,7 @@ function __phpunit_run_isolated_test()

$test = new {className}('{methodName}');

$test->setData('{dataName}', unserialize('{data}'));
$test->setData(unserialize('{dataName}'), unserialize('{data}'));

Choose a reason for hiding this comment

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

Why are serialize() / unserialize() now used for the name of a data set?

@WalterWoshid
Copy link
Author

Yes, I'll have a look at it :)

@sebastianbergmann
Copy link
Owner

Yes, I'll have a look at it :)

Any update on this? Thanks!

@sebastianbergmann
Copy link
Owner

Yes, I'll have a look at it :)

@WalterWoshid Any update on this? Thanks!

$fileContents = '';

foreach ($tests as $test) {
$fileContents .= serialize(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why/whether it works to concat serialized strings together?

https://3v4l.org/t1ghY

@staabm
Copy link
Contributor

staabm commented Apr 18, 2024

@WalterWoshid do you have plans to finish this 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.

None yet

3 participants