Add first "system level" (full-scale integration) tests

[mpdude]

This adds three "system" level tests for problem areas I am aware of:

• Analyzing a project with PHP 7-specific code with Rector running under PHP 8
• Analyzing PHP 8 code with a Rector running PHP 7
• Autoloading or even accidentally executing project code due to autoloader mess-ups between Rector and the project's codebase.

These tests do a full-scale integration, i. e. they will run bin/rector as separate processes for all fixture projects below tests/system-tests.

tests/system-tests/run-tests.sh is provided to run all these tests in succession.

[mpdude]

@TomasVotruba regarding #5775 (comment) – is this heading in the right direction?

[TomasVotruba]

@TomasVotruba regarding #5775 (comment) – is this heading in the right direction?

I thought the problem was about Docker and mix of code version vs Docker version. But I don't see any Docker here

[TomasVotruba]

This adds three "system" level tests for problem areas I am aware of:

Better start with 1 problem, so it's easier to review and finish. Then we can move to another. Otherwise it can be too complex to review and become stale. Then closed few weeks later.
See: https://matthiasnoback.nl/2021/02/refactoring-prepare-to-stop/

[mpdude]

Ok, let's try to sort out things step by step.

In this comment, you wrote:

Scoping is still needed. There are some rare cases, like annotation parsing or native method reflection, that require native class to be loaded.
But that's only guess... It would be better to have this covered by test case right here, e.g. installing Symfony Console 2.8 on PHP 5.6 with one simple Command implementation and running PHP 8 Docker image on it.

This is what this PR is about: Let's have a minimalistic "project" or codebase that uses Symfony Console 2.8, or that uses PHP 7-specific language syntax, or that uses PHP 8-specific language syntax.

And then let's run pure (unscoped) Rector on that code, using different PHP versions (7.3, 8.0, ?), to find out what happens.

Do you agree so far?

[TomasVotruba]

And then let's run pure (unscoped) Rector on that code, using different PHP versions (7.3, 8.0, ?), to find out what happens.

That would not work, as scoped version is also to avoid composer install conflicts. E.g. try this one:

composer require symfony/console:2.8
composer require rector/rector:0.9

See https://getrector.org/blog/2020/01/20/how-to-install-rector-despite-composer-conflicts

[mpdude]

Seems I need to work on my explanation skills, I cannot get the point across 😔. Yes, there was room for misinterpretation and you found that weak spot...

Let me try again.

Yes, when you install Rector via Composer directly into your project, Composer will resolve package versions that work for both the project and for Rector. Or Composer will abort with an unresolvable conflict. So we cannot install Rector + a conflicting version e. g. of symfony/console directly this way through Composer.

But lets look at another usage scenario where Composer resolution does not happen that way, and that's when running the Docker image. In this case, Rector in the Docker image brings along its own set of dependencies (i. e. vendor). And the project has a vendor folder that was built without taking Rector into consideration. Now when Rector also activates/used the project's autoloader, there may be surprising side effects or crashes, for example #5003.

The problem here is not due to Docker: We have the same situation if you clone the rectorphp/rector repo somewhere, run composer install, and then you change into your project's directory and run path/to/rector/bin/rector .... It's the same that happens when you run the Docker image, but without Docker 😉. (By the way, my guess would be .phar brings very similar challenges, but let's leave that aside, ok?)

You agree so far?

[TomasVotruba]

I'm not sure I see an issue. So this would not happen in Docker, but there is no Docker test in this PR. What is the point of such test that does not test a Docker then?

The failing GitHub Action would tell it more clearly :)

[mpdude]

Let's put it this way: This PR is not about an issue, but it adds a few tests that document and guard a very important result you achieved with the static reflection PR (#5665).

To better illustrate this, I have ported the tests from this PR here to the commit immediately before you merged #5665. You can see the result in this action run in my fork. They all fail, in part for different reasons.

• On all PHP versions, Rector was not able to clearly separate its own autoloader (and thus, its own code it needs to run) from the project's autoloader. It might happen that it would run code from a directory that should be treated entirely as data (input) for analysis/processing. (This is the code, here is an example failure).
• Rector running on PHP 7.3 failed when analyzing PHP 8 code
• Rector running on PHP 8 failed when analyzing PHP 7 code.

But now, with #5665 (and I believe also #5772), this is no longer the case. All of those tests pass, here is the PHP 8 example. 🥳

So, what can we learn from all that?

1. Running Rector from a source checkout (git clone ...Rector..., composer install, cd ~/your-project, ../path/to/rector/bin/rector ...) and running from a Rector Docker image will no longer mix up autoloading from Rector's vendor with autoloading from the codebase being analyzed. Rector will not load or even execute project code ("data") in its own PHP process.
2. So it was probably a safe decision to drop the secured Docker image variant (Drop no longer needed secured rector version of Docker image #5759)
3. Scoping/prefixing is no longer necessary for Rector to put its own dependencies into a safe namespace (so to say) when running from Docker.
4. We do not anymore need the PHP (major) version running Rector to be the same as the PHP (major) version being used in the project.

(Unless, of course, you happen to know edge cases where 1. is not true? 🤞🏻)

If we can agree that these points 1. – 4. are correct, then

• We might have addressed this remark?
• Many (if not all?) caveats I documented in Document current limitations #5678 have been resolved; maybe I can close that PR entirely (I'll see).
• Through Build just one Docker image with PHP 8 and without scoping #5775, we can
  • simplify the Docker build process a bit,
  • remove redundancy (keep this, drop that)
  • make end users' life easier: No need to think about which Docker image variant to use if there is only one (PHP 8.0),
  • move contents of the Docker image closer to a pristine "source" checkout: Maybe that might ease debugging/support or avoid potential problem areas.

What do you think?

[TomasVotruba]

I'm sorry, I have troubles with long texts with dozens of links to understand. I'm unable to process this.

Could you process the comment I made so we have the failing output in CI?

• Add first "system level" (full-scale integration) tests #5784 (comment)
• Add first "system level" (full-scale integration) tests #5784 (comment)
• Add first "system level" (full-scale integration) tests #5784 (comment)

[mpdude]

Sorry, how impolite of me 🙈.

I tried to answer your comments as briefly as possible.

There is no failing test here. This PR adds a few ✅ tests for important results of your #5665.

If we take the same tests, but run them just before you merged #5665, there are failures: https://github.com/mpdude/rector/actions/runs/639331588

[TomasVotruba]

Before any further review from my side, the commens above must be resolved. It does not make sense to me to give the same feedback again and drag in circles.

[mpdude]

@TomasVotruba I think finally understood your remarks above. System tests are now a dedicated workflow, vendor/ inside the fixture directories is set up during the action run. That makes it much simpler and fewer files in this PR.

[mpdude]

Rebased, all checks are now ok.

@TomasVotruba did I resolve your comments appropriately?

[TomasVotruba]

Thanks for simplification 👍

Now it's much clearer what is the goal of this. It seems like e2e tests - we definitely need that 👍

This repository is split-only after last weekend. The development is moving to rector-src, see https://getrector.org/blog/prefixed-rector-by-default for more.

I'll move this PR manually there and test it locally, to save your the booring work. I think you've invested already enough time and energy. Thank you for that 🙇

[TomasVotruba]

It's up and running: https://github.com/rectorphp/rector/runs/2555050237

👏

[TomasVotruba]

I'll look into way how to make them parallel in CI, so CI fails on the broken test not for all.