run-tests.sh - segregate command line parsing and use Symfony Console classes

I've not done a full review, but the first thing that struck me were the many changes from $args('the-string') to $input->getOption('the-string'). In fact there are 73 of these, and I suggest it would be more readable if the $args array was retained, and simply built by the necessary calls to $input->getOption() but doing each only once. This would also reduce the compexity of the MR, and make it possible to print the entire $args array when doing debug. But having looked now at more of the changes, maybe my idea is not so important and stick with what you have done. But the diff is massive and needs a lot of checking. I can try to use this in a Contrib pipeline to test, but I expect the Core tests are doing enough to verify it already.

Adding to meta parent

Unblocking this, now that #3515347: Reduce run-tests.sh complexity in spawning subprocesses has been done. Thanks for workig on that one, it's good to have some progress.

From #7

"#3515347: Reduce run-tests.sh complexity in spawning subprocesses" would need to be done prior ... as that will remove some of the CLI arguments and make this one simpler.

Is that what the most recent commit does?

I will also use this MR in a contrib test, but as I said in #6 if the Core tests run OK then that's probably enough?

I've tested this in a Contrib CI pipeline, using the diff from this MR to patch run-tests.sh. If anyone else would like to use this I have attached the addition for your .gitlab-ci.yml file. I think it needs to be used in the 'next minor' variant, which runs 11.4-dev as the patch might not apply in 11.3.2

Here's the pipeline, running one functional test, one functional javascript and one kernel test
https://git.drupalcode.org/issue/scheduler-3356800/-/pipelines/727371

I've compared the three logs with the regular non-patched version, and they are identical in output. The only change we see is that the php binary is now shown as /usr/bin/sudo wheras before it was /usr/local/bin/php

--------------------------------------------------------------
Drupal Version.......: 11.4-dev
PHP Version..........: 8.3.30
PHP Binary...........: /usr/bin/sudo
PHPUnit Version......: 11.5.50
PHPUnit configuration: /builds/project/scheduler/web/core
Database.............: MySQL, Percona Server, or equivalent
Database Version.....: 8.0.44
Working directory....: /builds/project/scheduler/web
--------------------------------------------------------------

I have also run it with the addition of --sqlite ":memory:" as requested.
Pipeline https://git.drupalcode.org/issue/scheduler-3356800/-/pipelines/728118 shows it is used in the command here. Again the three output logs are identical to before.

Thanks for the info. The Gitlab CI job has --php "$(which php)" in the command line when executing run-tests.sh, so it sounds like this is redundant with this new chnage?. This part of the job template is used for all variants, so it may be the case that we leave it in for now, because these run-tests changes won't be back-ported to Drupal 10. Or we could check the core version and add that snippet conditionally, which might be a better solution.

I've tested the updated MR, and also removed the --php parameter, and now we get back the original
PHP Binary...........: /usr/local/bin/php
https://git.drupalcode.org/issue/scheduler-3356800/-/jobs/8289782#L593

The endgame is the same, but behind the scenes it's no longer the input resulting from --php "$(which php)", it's the output of PhpExecutableFinder::find()

Yes, that is what I meant. All's good.

You also asked for manual testing of --list and --list-files. They both give the results for all tests in all modules, as intended. But adding --directory=modules/mymodule does not reduce the output, we still get everything. This is not only very slow, but also (as reported elsewhere too) if any other 3rd-party contrib project causes deprecations due to missing classes, then the whole command fails. It would be great to have --directory= not ignored in these cases. But maybe that work will be done in #3549601: Allow --directory and @group to work together in run-tests.sh next, after this issue.

The --help formatting is greatly improved. Much better use of the screen give more info without scrolling. I have also checked the text changes, and apart from moving some options, and the extra info such as defaults, these are the differences:

• new options --phpunit-configuration and --ci-parallel-node-total
• deprecated --php
• in --sqlite, removed:
  Note that ':memory:' cannot be used, because this script spawns sub-processes. However, you may use e.g. '/tmpfs/test.sqlite'
• file option - removed the text This argument must be last on the command line.

So that all looks good.

@jonathan1055 would you say this one is ready?

I have done the Contrib testing, both in Gitlab pipelines and locally, and all seems OK. However, I don't pretend to understand the actual coding changes that Mondrake has made. I am assuming that if the core tests run, and contrib too, then in principle it is working.

Just a couple of points:

1. From the discussison in #6 and #7 it would be good to get that follow-up issue created and linked from here.
2. In #16 Mondrake said: "I think there's other code that may be dead now, will try to check on it later ..."
   It would be good not to lose sight of this. I don't know if that is a separate issue or not.
3. I have made a couple of minor suggestions in the MR thread regarding text of the help. There were a few very long lines which wrapped and made it harder to read. I know we can't predict the terminal window size, but at least these few line-breaks will help.

NW for these things, but otherwise if Mondrake is confident then yes lets get this moving onwards :-)

Thanks for the info and commit links, and for adopting my suggestions. The help looks good now.

I had not tested the --class option before, so now have done that. Two small observations

1. I found that using just the final class name was not enough, it needs the full path. For example --class MigrateSchedulerDataTest fails to find the class, but --class 'Drupal\Tests\scheduler\Kernel\MigrateSchedulerDataTest' does work. If this is intended then maybe the help should state that?
2. I also found that quotes are required around the class, which is OK, but the example in the help does not show quotes. Quotes are not needed when using the --file option. I've made a MR suggest for this.

Then let's call this RTBC. I presume it is only for 'main' and 11.x, so will affect 11.4.x but not 11.3.x ? Just want to know what audience this could impact if any problem has not been noticed.

Thank you for the clarifications in the help. Yes you are right, quotes are not specifically required, that was just my first attempt at solving it and it worked. But backslahes also work, so it's good you have that in the help.

I will be brave and mark this RTBC, but I really do not know if we've tested every option. I guess that may become apparent if things break.

Do you have a plan for which branches this will be merged into? re my last question on #27.

[edit: the last addition to help needs a linebreak. I have added that in a MR suggestion]

I would be surprised if this one would get any further.

Good, that's what I was expecting.

Added some question/feedback, it's hard to know where we are done here, ideally one day this will be a fully fledged class implementation of a console command and this is a good step on the way.

I read the docs and I think we can set the defaults once, see MR comments.

I think that's fine, it feels like an error to specify the option without a value anyway?

This is interesting because it might converge with #3453474: CLI entry point in Drupal Core soon...

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Reviewed the MR, found a bug and a minor improvement that can be ignored if not desired. Feel free to self-RTBC afterwards.

Committed afcf201 and pushed to main. Thanks!

Not sure whether this one should go into 11.x as well. I see that the related issues were backported, so I'll ask around.

Discussed with release managers and we think it should be backported to 11.x.

Seems like a good rebase.

Committed 45c8034 and pushed to 11.x. Thanks!

Looks like the push didn't make it to 11.x.

Oops :)

Thank you for porting this back to 11.x. It is used in the 'Next Minor' Gitlab pipeline varaint, which uses 11.4-dev, so the changes will get used and tested for real.

I did notice something in the help, which might be a typo, or it might be my mis-understanding. The help for --suppress-deprecations says

Stops tests from failing if deprecation errors are triggered. If this is not set the value specified in the SYMFONY_DEPRECATIONS_HELPER environment variable, or the value specified in core/phpunit.xml (if it exists) will be used. The default is that any unexpected silenced deprecation error will fail tests.

In that last line, should it be "any unexpected unsilenced deprecation error will fail tests". If the deprecation is silenced then I would not want the test to fail. It's the unsilenced ones that we need to be alerted of. At least, that's my understanding. Please correct me if I'm wrong.