[CI] Use sudo -E to execute run-tests.sh

The MR shows permission denied failures: https://git.drupalcode.org/issue/drupal-3476189/-/jobs/2831557

-1 as this could hide real issues in file access. It is less than I originally thought (as the webserver is not running root part of functional tests will still be non-root) however that still leaves kernel and unit tests to run excessive permissions.

Sudo supports passing all environment variables using the -E flag. Tagging as needs an IS a update.

I noted in the linked issues that it appeared the problem was that run-tests.sh appeared to accidentally call sudo inside its loops (probably some how confused when looking at argv would be my guess) I would suggest core debug and fixing the underlying fault in run-tests.sh instead of switching to a privileged user for testing.

The MR made just shows the problem. It’s not the solution )or at least all of it). I agree that root should not be needed.

I think that we agree on the solution but maybe the issue description or title does not reflect that.

Few minutes with the debugger. Global $php is indeed being set to the sudo path.

I have not run a full test run however it appears the solution to the root fault is to move elseif ($sudo = getenv('SUDO_COMMAND')) { block before elseif ($php_env = getenv('_')) { in simpletest_script_init()

(check for sudo before checking if _ is set)

Edit: I have not validated how that change would work if a user does

sudo /bin/bash;
php run-tests.sh; 

SUDO_COMMAND will be set in the environment however not present on the command line of the call to run-tests.sh)

Counter view:

Generally you want all tests that execute code to run as close to production as possible, this includes unit and kernel tests. Both of these can access the file system and running as root can mask faults.

In production it is unlikely the process running any of the Drupal code will have root privileges.

All our other tests (cspell, phpstan, lint, etc) are not executing the code they are just reading it for analysis as such the execution user is not relevant to the code.

Fixed up the tests to pass in HOME to clean up the small few errors that remained.

The last two remaining errors are related to Telemetry. It appears that OTEL_COLLECTOR is being set to a literal string $OTEL_COLLECTOR on cursory glance I'm not 100% sure why.

Perhaps Gitlab does this when its an undefined value? Though I don't understand why it would work before the sudo. Unless this was kept in an environment file for user www-data we are no longer loading, although I don't see any code doing so on cursory glance either.

Really minor feedback in the MR. Two questions and a small character left.

I think this is a great improvement as we know can pass all variables via -E instead of needing to pass one by one.
There is some really minor feedback on the MR, some of it already addressed and some of it might just be a non-issue at all.

This change will also simplify the solution for the gitlab_templates related issue.

I'm setting it to RTBC.

IMHO this needs backport to all branches currently tested so the parallel contrib issue can benefit as well

I will note gitlab_templates probably should just use the —php flag for run-tests.sh as it allows them to support versions this can’t be backported for (existing tagged releases) for those who may do min/max testing.

They could do the above today without waiting for this issue.

Everything looks in order here, no unanswered questions.

Left one question on the MR

Maybe instead of using such a long sudo command it's better to add simple wrapper-script to source .test.env before running the command.
Moreover it will make logs shorter and less noisy to parse!

Also it should be enough to use su for this purpose so more secure environment

Now we use sudo -u www-data -E HOME=/var/www to set the correct home directory path, but sudo provides also -H, --set-home option that does the same, so maybe let's use it instead?

I pushed the changes, please review.

Re OTEL_COLLECTOR this is injected into the scheduled core performance test pipelines directly in GitLab.

There is also code in PerformanceTestTrait::openTelemetryTracing() that skips sending to the collector if the variable is not set:

    $collector = getenv('OTEL_COLLECTOR');
    if (!$collector) {
      return;
    }

The scheduled performance runs ensure that we get precise timings recorded regularly. Outside of those, the same tests should still run without the collector as they perform various assertions about numbers of queries, etc, and it is important to know for every MR whether we have affected cold/warm cached page loads.

I did a light review of the comment and I don't see any response to the suggestions by andypost in #24.

Added a comment about OTEL_COLLECTOR, setting it to the empty string is a bit confusing when we shouldn't need it at all - I think we can remove it entirely.

I think we only need this on 11.x, it doesn't backport cleanly to 10.x either. Please open a backport MR if you disagree!

Committed eba0136 and pushed to 11.x. Thanks!