Fixed before and after class methods also called from primary process when test class or method is run in separated process. #6041

akotulu · 2024-11-19T03:50:39Z

Our project uses heavily define constants in multiple sub modules and integration tests are ran inside separated test class process using RunClassInSeparateProcess attribute. Every TestCase sets its own defines (constants) in setUpBeforeClass, but PHPUnit calls the function on every registered TestCase from primary process thus failing to initialize sub module resources after first call, and primary test process receives an exception (e.g. invalid path).

// Define application path
define('APPLICATION_PATH', realpath(PROJECT_APPLICATION_PATH . '/module/xxx'));
// Define assets' path
define('ASSETS_DIR', realpath(__DIR__ . '/../assets'));
// Define server name
define('SERVER_NAME', 'xxx.abc.ee');
// Boot
include __DIR__ . '/../../../boot.php';

Before and after class methods must not be called at all from primary process if the tests are ran inside separated process (sandbox).

Also there is a issue with the src/Framework/TestRunner/templates/class.tpl. Idk if the parameter RunClassInSeparateProcess isn't properly implemented, but the template is for sure invalid when set and setUpBeforeClass is called for every test defined in TestCase.

… when test class or method is run in separated process.

sebastianbergmann · 2024-11-19T04:20:24Z

Also there is a issue with the src/Framework/TestRunner/templates/class.tpl. Idk if the parameter RunClassInSeparateProcess isn't properly implemented, but the template is for sure invalid when set and setUpBeforeClass is called for every test defined in TestCase.

That would be #5230.

sebastianbergmann · 2024-11-19T04:22:33Z

Please target the 10.5 branch as this is the oldest branch that still receives bug fixes. And please add at least one test case that fails without the proposed changes but passes with them. Thank you!

…lved issue with after class method.

akotulu · 2024-11-19T06:41:10Z

I've added a test and fixed after class being called twice. How do I target this branch? Checked out only master.

codecov · 2024-11-19T07:02:56Z

Codecov Report

Attention: Patch coverage is 86.17021% with 13 lines in your changes missing coverage. Please review.

Project coverage is 94.69%. Comparing base (0fde984) to head (2fdaf3d).
Report is 45 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Framework/TestSuite.php	87.50%	10 Missing ⚠️
...Framework/TestRunner/SeparateProcessTestRunner.php	78.57%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #6041   +/-   ##
=========================================
  Coverage     94.69%   94.69%           
- Complexity     6456     6463    +7     
=========================================
  Files           697      697           
  Lines         20307    20311    +4     
=========================================
+ Hits          19229    19234    +5     
+ Misses         1078     1077    -1

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

mvorisek · 2024-11-19T15:24:18Z

tests/end-to-end/sandbox/_files/BeforeAndAfterClassMethodCallCountTest.php

+
+//#[RunTestsInSeparateProcesses]
+#[RunClassInSeparateProcess]
+final class BeforeAndAfterClassMethodCallCountTest extends TestCase


Simillar test would be good for a class with some tests run in a separate process and some not.

These tests will fail cause before and after method will be called multiple times when TestSuite is executed in another process. Purpose of this fix was to prevent main process executing before and after class methods when attribute is set. I don't have atm time to fix the issue with process isolation executing these functions multiple times, it requires a bit of redesign.

Sure, I mean to have simillar test for all usecases - for RunClassInSeparateProcess attribute when all tests are run in a separate process and also when tests within a class are run in a separate process only partly.

…d test methods.

akotulu · 2024-11-20T03:22:42Z

I've fixed all the issues with separated process handling. Re-implemented separated process class template and added extra tests to validate all the before and after class methods are properly called.

New class template is missing these setters. I don't know if they are needed. Please comment

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

staabm · 2024-11-20T10:27:38Z

src/Framework/TestRunner/templates/class.tpl

@@ -69,52 +72,67 @@ function __phpunit_run_isolated_test()

    ErrorHandler::instance()->useDeprecationTriggers($deprecationTriggers);

-    $test = new {className}('{name}');
+    ini_set('xdebug.scream', '0');


is this line fixing a separate problem and should be separated from this PR?

Well problem began with PHPUnit calling before and after class methods from primary process when run in separated process. After fixing the issue I stumbled on another one where those methods are called for every test while RunClassInSeparateProcess is defined. So yes, this PR contains fixes to before and after class methods being falsely called while using RunTestsInSeparateProcesses, RunClassInSeparateProcess and RunInSeparateProcess attribute.

Both of those templates (class.tpl, method.tpl) where diff identical and class template was invalid for those cases and causing undesired behavior.

To be more clear: my question was about this specific xdebug line.

Was this moved from elsewhere or is this a line we need for the after/before methods?

This line was in the source before my redesign. Copied common functionality from previous version.

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

These lines I don't known what they do. Are they for proxing primary process global state?
Our project now runs all the tests properly without any issues. We have suites without and with separated process. Also mixed tests.

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

These lines I don't known what they do.

They pass data from data providers and depended-upon tests to the test.

sebastianbergmann · 2024-11-21T06:38:44Z

Please fix the coding guidelines violations.

akotulu · 2024-11-21T22:38:34Z

Weird behavior when project is pulled on Windows. php-cs-fixer tool reports all the project files having all kinds of issues. Had to pull the project inside wsl. CS issues have been fixed.

sebastianbergmann · 2024-11-23T09:09:50Z

This pull request has grown quite a bit since it was first proposed, making it hard to review. For instance, the change from an array to an object structure seems to be unrelated to problems that you want to address. And "problems" is a problem: this pull request addressed multiple problems. It would be easier to review smaller, more focused pull requests.

akotulu · 2024-11-23T19:10:54Z

Well this pull request can't be split cause changes are related to each other. Running tests in a separated process would have returned an array containing arrays with tests information. To distinguish it from method template returning test data in an array, I changed returning array to object in both templates. Tests in separated process return array containing objects and method returns object.

sebastianbergmann · 2024-11-24T06:32:40Z

Please have a look at the failing tests.

akotulu · 2024-11-25T00:37:05Z

Added a check for skipped and incomplete tests. This 'improvement' is kinda as is and can't be done the right way at the moment. TestBuilder is loading all the tests to the TestSuite in all cases from primary process. Loading tests step should come after analyzing the test file for attributes so TestRunner can be executed for a specific test file in either same or separated process and these checks with the boolean values can be removed. This would simplify a lot of unnecessary code. Also TestStatus class is kinda of overhead, you can't check for same class instance without using string ::class or static variable.

sebastianbergmann · 2024-11-25T06:37:56Z

Thank you for looking into the tests. However, I am unable to review this pull request which seems to get bigger and bigger.

akotulu · 2024-11-25T07:14:36Z

Bigger? 5 files have changed + tests.
Changes:

Rewrote class.tpl template to support running whole test file in separated process;
Changed method.tpl returned array to object;
Changed src/Framework/TestRunner/SeparateProcessTestRunner.php function processChildResult to support reading an array of results;
In TestSuite added two booleans, changed invokeMethodsAfterLastTest and invokeMethodsAfterLastTest to public (moved private mothods to bottom);
Fixed your tests/end-to-end/regression/5884/tests/FooTest.php not unlinking temp files when test fails by moving assert to end of test;
Added new test directory called sandbox with 3 new tests;

If this is counted a big change then I don't know what is a small change. This change cannot be made smaller cause class.tpl wasn't actually implemented. Someone copied it from method.tpl and counted it as a day.

staabm · 2024-11-29T12:50:30Z

src/Framework/TestRunner/SeparateProcessTestRunner.php

            }

-            Facade::instance()->forward($childResult['events']);
-            PassedTests::instance()->import($childResult['passedTests']);
+            foreach ($childResult as $result) {


Could this array to object transformation be extracted into a separate refactoring PR, without the actual bugfix?

I try to find ideas on how we can make this PR smaller and manageable

Fixed before and after class methods also called from primary process…

aeea56a

… when test class or method is run in separated process.

sebastianbergmann added type/bug Something is broken feature/test-runner CLI test runner feature/process-isolation Issues related to running tests in separate PHP processes labels Nov 19, 2024

Implemented test for process before/after class method calls and reso…

ff23972

…lved issue with after class method.

mvorisek reviewed Nov 19, 2024

View reviewed changes

Resolved TestSuit separated process (sandbox) handling for classes an…

c494e99

…d test methods.

staabm reviewed Nov 20, 2024

View reviewed changes

Fixed coding guidelines violations.

6a94ab8

Fixed results not returned properly for skipped tests.

2fdaf3d

sebastianbergmann force-pushed the main branch from da21665 to 4dddc8c Compare November 28, 2024 09:34

staabm reviewed Nov 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed before and after class methods also called from primary process when test class or method is run in separated process. #6041

Fixed before and after class methods also called from primary process when test class or method is run in separated process. #6041

akotulu commented Nov 19, 2024 •

edited

Loading

sebastianbergmann commented Nov 19, 2024

sebastianbergmann commented Nov 19, 2024

akotulu commented Nov 19, 2024

codecov bot commented Nov 19, 2024 •

edited

Loading

mvorisek Nov 19, 2024

akotulu Nov 19, 2024 •

edited

Loading

mvorisek Nov 19, 2024

akotulu commented Nov 20, 2024 •

edited

Loading

staabm Nov 20, 2024

akotulu Nov 20, 2024 •

edited

Loading

staabm Nov 21, 2024

akotulu Nov 21, 2024

sebastianbergmann Nov 21, 2024 •

edited

Loading

sebastianbergmann commented Nov 21, 2024

akotulu commented Nov 21, 2024

sebastianbergmann commented Nov 23, 2024

akotulu commented Nov 23, 2024 •

edited

Loading

sebastianbergmann commented Nov 24, 2024

akotulu commented Nov 25, 2024

sebastianbergmann commented Nov 25, 2024

akotulu commented Nov 25, 2024 •

edited

Loading

staabm Nov 29, 2024 •

edited

Loading

Fixed before and after class methods also called from primary process when test class or method is run in separated process. #6041

Are you sure you want to change the base?

Fixed before and after class methods also called from primary process when test class or method is run in separated process. #6041

Conversation

akotulu commented Nov 19, 2024 • edited Loading

sebastianbergmann commented Nov 19, 2024

sebastianbergmann commented Nov 19, 2024

akotulu commented Nov 19, 2024

codecov bot commented Nov 19, 2024 • edited Loading

Codecov Report

mvorisek Nov 19, 2024

Choose a reason for hiding this comment

akotulu Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

mvorisek Nov 19, 2024

Choose a reason for hiding this comment

akotulu commented Nov 20, 2024 • edited Loading

staabm Nov 20, 2024

Choose a reason for hiding this comment

akotulu Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

staabm Nov 21, 2024

Choose a reason for hiding this comment

akotulu Nov 21, 2024

Choose a reason for hiding this comment

sebastianbergmann Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

sebastianbergmann commented Nov 21, 2024

akotulu commented Nov 21, 2024

sebastianbergmann commented Nov 23, 2024

akotulu commented Nov 23, 2024 • edited Loading

sebastianbergmann commented Nov 24, 2024

akotulu commented Nov 25, 2024

sebastianbergmann commented Nov 25, 2024

akotulu commented Nov 25, 2024 • edited Loading

staabm Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

akotulu commented Nov 19, 2024 •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading

akotulu Nov 19, 2024 •

edited

Loading

akotulu commented Nov 20, 2024 •

edited

Loading

akotulu Nov 20, 2024 •

edited

Loading

sebastianbergmann Nov 21, 2024 •

edited

Loading

akotulu commented Nov 23, 2024 •

edited

Loading

akotulu commented Nov 25, 2024 •

edited

Loading

staabm Nov 29, 2024 •

edited

Loading