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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

wants to merge 1 commit into from

Conversation

WalterWoshid
Copy link

@WalterWoshid WalterWoshid commented Feb 22, 2023 •
edited
Loading

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
This was referenced Feb 23, 2023
Closed
Closed
@codecov
Copy link

codecov bot commented Feb 23, 2023 •
edited
Loading

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 WalterWoshid changed the base branch from main to 10.5 January 16, 2024 14:37
@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!

sebastianbergmann
sebastianbergmann reviewed Jan 17, 2024
View reviewed changes
src/Util/PHP/Template/TestCaseMethod.tpl
@@ -16,7 +16,7 @@ if (!defined('STDOUT')) {

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

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?

sebastianbergmann
sebastianbergmann reviewed Jan 17, 2024
View reviewed changes
src/Util/PHP/Template/TestCaseMethod.tpl
@@ -50,7 +50,7 @@ function __phpunit_run_isolated_test()

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

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

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!

staabm
staabm reviewed Mar 16, 2024
View reviewed changes
src/Util/PHP/Template/TestCaseClass.tpl
$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

@sebastianbergmann sebastianbergmann mentioned this pull request Apr 14, 2024
Open
@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
Reviewers

@staabm staabm staabm left review comments

@sebastianbergmann sebastianbergmann sebastianbergmann left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@WalterWoshid @sebastianbergmann @staabm