BROWSERTEST_OUTPUT_VERBOSE is not passed to run-tests.sh when running with it

I think this is the same as ✨ Do not run commands as other user as we lose env variables Active .

I tried back in the day and something didn’t quite work (I can’t remember what exactly).

I’ll review this one anyway, just wanted to link the related issue as they might be duplicates.

Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3475974/-/pipelines/28...

It’d be great if we have a before and after on another project where this is demoed.

We still have the same issue as the one mentioned back in the day. See this https://git.drupalcode.org/project/api/-/jobs/2821944

I completely forgot about the existing issue, sorry.

What’s strange here is that the use of -E works with phpunit cli but not with run-tests.sh. Both spin child subprocesses, but the error is only shown by the latter.

FTR I found this to pass the pwd to sudo, but do not know if it can be public: https://unix.stackexchange.com/questions/434759/whats-the-purpose-of-usi...

Maybe dropping usage of sudo is just the easiest way around? I am willing to help testing it in Image Effects if that helps

I confirm the same problem applies to core CI, BTW: you cannot use the variable in MRs to silence the HTML output.

Similarly, we tested the very nice --testdox option in #3359927: Document option to improve phpunit information in log → but this also only works with concurrent=0 executing phpunit directly.

@jonathan1055 re #9, that would require changing run-tests.sh to accept a similar argument and pass it on to it's own spawned PHPUnit CLI executions. I trust we'll get there, but that's a core issue first.

mondrake → changed the visibility of the branch 3475974-1 to hidden.

MR!259 looks more promising in removing usage of sudo for calling run-tests.sh and phpunit. To solve the permission issues, we add the www-data user to the root group.

See usage of the MR in Image Effects issue 📌 Deprecation testing Active , MR https://git.drupalcode.org/project/image_effects/-/merge_requests/61.

With this MR,

running via run-tests.sh: https://git.drupalcode.org/project/image_effects/-/jobs/2823311

running via phpunit cli: https://git.drupalcode.org/project/image_effects/-/jobs/2823743

This is looking good. I ran the downstream pipelines and they all seem good too.
RTBC. Thanks!!

Merged. Thanks.

Updated title and proposed resolution.

Actually, I reverted it based on this comment https://www.drupal.org/project/gitlab_templates/issues/3400372#comment-1... ✨ Do not run commands as other user as we lose env variables Active , as the raised concern is not addressed.

Before fully committing this, I'd like to at least see the options and to have some extra conversation about it.

fjgarlin → credited cmlara → .

https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/261... has what was merged and then reverted.

Note that the -E option works with vendor/bin/phpunit but not with php run-tests.sh.

#21 fair enough.

In fact, I think we should just be running tests as root, like we run all other tasks in these jobs. www-data is, or at least it should be, the user for the Apache webserver to deliver pages: using it to run Unit or Kernel tests is just, IMHO, wrong, and possibly a remnant of simpletest times. Functional(Javascript) tests is different, and IMHO the test should run under root, and the SUT under www-data because these tests check the results of pages delivered by the webserver.

Now, www-data fails some file permissions in the SUT, which it should not IMHO. This is clearly visible if you remove the usermod bit and run any functional test - the failure is on the SUT and passed on to the test. For this purpose I added the user to the root group.

Possibly, we should look for a fix in core and have test run without any usermod but that is probably going to take some time.

I agree that adding the user to the root group is the easiest option, as that way all env variables are there. I also agree that this is something that probably should be fixed on core, but that might take longer.

So, my suggestion is:
- Create the core issue so we can also make reference to it here.
- Create a new toggle variable _PHPUNIT_ROOT_MODE (or something that makes sense), which can be turned on (1) by default.
- Implement the solution in the MR when _PHPUNIT_ROOT_MODE=1. Default to the existing behaviour when _PHPUNIT_ROOT_MODE=0.

This way, if there are concerns about running the tests as root, maintainers can still turn it off.

If the change is fixed/merged into core and we no longer need this, then we can clean up the code and deprecate the variable.

In any case, happy to hear any other thoughts.

#28 opened 📌 Drop using sudo to execute run-tests.sh Active in core issue queue. The rest of the plan makes sense to me, thanks!

Thanks for that. I added a quick MR to the core issue that shows the problem.

I implemented the approach from #28 in the MR and it's ready to review. It'd be great to run it both with the new variable turned on and off.

Downstream pipelines: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/290463

New dowstream pipelines: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/290474

This seems supersafe now, with the possibilty to revert the change on-the-fly.

mondrake → changed the visibility of the branch 3475974-2 to hidden.

Maybe better postpone on the core issue?

Agree, given that there is traction in the core issue and that if that's solved, it will simplify the code here, I'd say let's wait on it (and maybe review/test there).

Worst case, if it's delayed, we have a plan B waiting in the MR.

Fix typo in title

With just sudo -E we have a problem that the home directory ($HOME) is set to /root that gives problems with access, because the dir has 700permissions.

But sudo provides also -H, --set-home option that does the same, so let's use it too? I'll push a commit with this change to the MR.

By the way, maybe instead of running the Nightwatch (and phpunit too) using sudo -U www-data, it'll be better to switch the user to www-data in the container, and perform all other commands as the www-data user?

And if we really need to do some actions as root, we can use sudo -E prefix for such commands.

This is a common approach in many pipelines to switch the container to the non-root user, and use sudo when we need root actions.

it'll be better to switch the user to www-data in the container

Would probably be a BC break as any before_script work would then need to make itself accessible to www-data, or would need to know to sudo if relevant.

Btw reiterating what I put in the core issue: gitlab_templates does not need to postpone on core to fix the sudo issue, just pass the PHP flag in to run-tests and sudo will work today. Gitlab_templates will need to do that anyways as there is no way core issue is being back ported to D9 (which some modules still test against). Just not sure if that should happen in this issue or one of the previous issues (since running as root has been shown to no longer be needed).

@cmlara - does the current MR in this issue match what can be done today? Or is something else different needed?

The current MR tries to move to running the tests as root rather than solely fixing the run-test problem that has stopped us from using -E in the past (as noted in core issue and elsewhere running as root can hide code faults)

Opened !293 with the minimal changes based on the previous few times this discussion occurred from ✨ Do not run commands as other user as we lose env variables Active and related, along with the the new info from the core issue allowing tests to remain as www-data yet pass through the needed environment variables.

it'll be better to switch the user to www-data in the container

Would probably be a BC break as any before_script work would then need to make itself accessible to www-data, or would need to know to sudo if relevant.

Yes, I fully understand that this is a breaking change, but still, it's for the great future, right? :) So, I filled a separate feature request with this idea: ✨ Switch drupalci/php-8.3-ubuntu-apache container to use www-data user instead of root Active

Thanks for putting the MR together.

Downstream pipelines for MR293: https://git.drupalcode.org/issue/gitlab_templates-3475974/-/pipelines/34...
The code there is way cleaner and we are not introducing any more variables/logic, so if that fixes the issue, I'd defo prefer that.

We might need to extend things to the test-only job and perhaps the nightwatch ones, so setting to "Needs work" based on that, but it's a clean and great approach.

D8+ changes are up. Do we want this to target back to D7?

Tested with PHPUNIT_CONCURRENT:0 with all available variants:
https://git.drupalcode.org/project/scheduler/-/pipelines/346310

and PHPUNIT_CONCURRENT:2
https://git.drupalcode.org/project/scheduler/-/pipelines/346769

All runs OK, but I do not have any deprecations to check against. I can add one back in to check the SYMFONY_DEPRECATIONS_HELPER value is being used.

mondrake → changed the visibility of the branch 3475974-pass-variables to hidden.

All runs OK, but I do not have any deprecations to check against. I can add one back in to check the SYMFONY_DEPRECATIONS_HELPER value is being used

FunctionalJavascript is also controlled by one of the pass through variables.

Tested MR293 in https://git.drupalcode.org/project/image_effects/-/pipelines/346788

That pipeline specifies BROWSERTEST_OUTPUT_VERBOSE: 'false' and it's visible in the logs that individual links to HTML output pages are no longer shown.

Thanks!

Actually the issue needs a new title as it's saying to do something we are not doing here

Deprecations via SYMFONY_DEPRECATIONS_HELPER works too
https://git.drupalcode.org/project/scheduler/-/jobs/3449385

I'll pick this up from here

Hi @lavanyatalwar, welcome to the issue and to Drupal.
There is not much more to do, so it should be nice and easy :-) I think the only things still to do was to accept the "apply suggestions" in the MR and also @moondrake wanted to change the issue title. Then it needs a full review by fjgarlin.

Thank you, @jonathan1055
I have applied the suggestions from the MR.
Kindly review it.

@lavanyatalwar Do you see that the pipeline failed? If you look at the log you can see that the 'check versions' job failed. This is because the issue fork does not have the latest commit(s). At the top of the MR you can see "The source branch is 1 commit behind the target branch.". The easiest way to resolve this is to enter a single comment using the keyword /rebase and this will trigger a rebase to include the missing commit.

I've changed the title to something that I think indicates better what this does. I'm setting to RTBC based on the runs in the downstream pipelines and the tests made in #50 and #52.

Re-tested after latest changes and all is still good
https://git.drupalcode.org/project/scheduler/-/pipelines/349271

Updated IS to add the actual solution

Merged. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.