Resolved test suite run in separated process executed one by one in separated process.

[akotulu]

Second pass of resolving RunClassInSeparateProcess attribute issues running TestSuite tests one by one in a separated process and calling before and after class methods for each test in the suite. Also test suite in separated process called primary process before and after class methods.

Created separated branch, it contains first pass modifications as well.

[staabm]

why have this methods been moved in this file? this makes the actual diff hard to read (in case this has been done by CS-fixer, it might make sense todo the private->public transformation in a separate PR, so the rest gets readable)

[akotulu]

It was done by CS-fixer. Only changed the access type.

[staabm]

I see, thx. I have this CS-fixer rule :)

[staabm]

@sebastianbergmann wdyt about this CS fixer rule? :)
(most of the diff in this file of the PR is generated by the CS fixer sorting methods arround, because they changed the visibility. we don't see the actual changes anymore)

[staabm]

saying just that, maybe @akotulu could use separate commits for the CS fixing changes and the rest, so we could review commit by commit

[sebastianbergmann]

I agree that there is a conflict of objectives at play here: one the one hand, I want methods to be ordered in a certain way. On the other hand, the proposed changes should be readable. In this case the method ordering gets in the way of patch readability because the visitbility of a method is changed.

[akotulu]

Ok, how can I revert it or close this branch and add the modifications without CS fixer?
Hmm, atm I will revert the function back and add the modifications. Rebase and commit to this pull request. After successful review I will commit again with CS fixer?

[staabm]

please rebase

[akotulu]

Rebased to main, also reverted to CS-fixer changes. Do I need to make a new pull request from my main?

[sebastianbergmann]

Force-pushing the current state to this PR should be enough.

[akotulu]

Hope I did it right.

[akotulu]

Hey, any progress on the review?

[sebastianbergmann]

I am currently only able to work for very short periods of time (and should not even do that), see https://phpc.social/deck/@sebastian/113487079241563305. I will get to this as soon as I am able to and kindly ask for your patience.

[sebastianbergmann]

Can you have a look at the failing tests, please? Thank you.

[akotulu]

I've fixed the tests (lack of directory for temp files for some reason). Also I've improved the class.tpl file to support --filter option. Previously when class was ran inside separated process, every test method was executed. How and where should I push these changes?
I've added extra test for the --filter option as well.

[sebastianbergmann]

Force-pushing to this pull request should be fine.

[akotulu]

Done, hopefully didn't mess it up :)

[codecov]

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 95.05%. Comparing base (de81d0e) to head (bf7170b).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Framework/TestSuite.php	73.33%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6063      +/-   ##
============================================
- Coverage     95.08%   95.05%   -0.04%     
- Complexity     6738     6742       +4     
============================================
  Files           725      724       -1     
  Lines         21251    21255       +4     
============================================
- Hits          20207    20203       -4     
- Misses         1044     1052       +8     

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

[sebastianbergmann]

Let me begin by expressing my gratitude for your persistence in working on this. This must not have been easy, especially since you sometimes had to wait a long time for my feedback, as well as do extra work to iterate this pull request based on my feedback.

Looking at the end-to-end tests that your pull request adds to PHPUnit's test suite, I trust that the changes you propose will restore the originally intended behavior of #[RunClassInSeparateProcess]. I cannot help but doubt that this functionality has ever worked as intended at all. However, I am certain that it has been broken for a long time.

I am not comfortable with the increase in complexity that the proposed changes introduce into the template that is used to generate the code that is run in a separate process. I would much rather reduce the amount of code in these templates than add more code to them, since the code in these templates is not seen by static analysis and the IDE.

I am beginning to wonder if deprecating #[RunClassInSeparateProcess] in PHPUnit 12 and removing it in PHPUnit 13 would not be a better course of action.

Running all tests of a test-case class in a single separate process was intended as a trade-off between isolation and performance. Personally, I prefer isolation over performance. Combined with the added complexity to make #[RunClassInSeparateProcess] work, I do not think this is a feature worth fixing/having.

I have not made a decision yet, but wanted to give you an update sooner rather than later.

[akotulu]

Understand the trouble. This feature is for us and I think for others is needed cause our 'old' project contains micro modules which each has its own global defines. Running test suite for each module will define values for first module and next module wont behave as intended cause the defines cannot be redefined. We are atm moving away from old Zend Framework v1 but still this is a long process.