BTB: Add cronRun function

This moves the method to a trait, and adds it to BTB.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -2062,13 +2064,6 @@ protected function translatePostValues(array $values) {
-    $this->drupalGet('cron/' . \Drupal::state()->get('system.cron_key'));

+++ b/core/tests/Drupal/Tests/Traits/Core/CronRunTrait.php
@@ -0,0 +1,17 @@
+    $this->container->get('cron')->run();

Note that this change to run cron directly from the service doesn't lose any test coverage, as the CronRunTest class tests running cron via the browser.

Hmm. Not sure about those fails. I can reproduce some of them locally on 8.3.x w/o this patch...

Apparently there's something special happening when cron is run via the browser. This switches the new trait to behave as the old method did. Perhaps we should add a follow-up to figure out why the direct call to the service fails.

Oops.

It would be readily unit-testable if the trait were able to call the service directly. I'm not sure if we need to tackle that in this issue though, as that could be a follow-up. The implicit usage of the trait seems like sufficient testing for this pass?

I think I was over-thinking the way a test would look. That test works perfectly :)

I still cannot figure out why invoking cron from the browser has a different effect that calling the service directly.

+++ b/core/tests/Drupal/Tests/Traits/Core/CronRunTrait.php
@@ -0,0 +1,17 @@
+  protected function cronRun() {
+    $this->drupalGet('cron/' . \Drupal::state()->get('system.cron_key'));
+  }

we should avoid using \Drupal where we can to not set a bad example. We have the container on both classes using the trait, so this should be $this->container->get(..)

Meaning here we would just introduce the trait, but not have BrowserTestBase use it, and allow other tests to use it as-needed?

I've switched the trait to use the container instead. I also added a response assertion, as locally I was seeing failures when I had accidentally turned off the http server it was running on, and couldn't figure out immediately why the test was failing.

I was wondering whether the response code condition should be moved to the actual test of this method, so its not there everytime.

I could go either way on this. Having it in the trait makes failures more verbose if some other test is breaking the cron callback for whatever reason. If it's only in the test, then cron running could potentially silently fail in other tests. However, the old method did not have this check, and it's been fine thus far :)

Makes sense. This does everything except script conversion and follow-up. When adding the follow-up, we should also perhaps explore why calling the service directly behaves differently from using the page controller too.

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -1952,13 +1954,6 @@ protected function translatePostValues(array $values) {
-    $this->drupalGet('cron/' . \Drupal::state()->get('system.cron_key'));

+++ b/core/tests/Drupal/Tests/Traits/Core/CronRunTrait.php
@@ -0,0 +1,17 @@
+    $this->drupalGet('cron/' . $this->container->get('state')->get('system.cron_key'));

There is a whole debate going on about the use of $this->container vs \Drupal in tests... Can we not make this change here. The debate is not yet finished and the way forward is unclear. See the latest comments on #2066993: Use \Drupal consistently in tests.

Changing the line back to use \Drupal is a novice task.

Adding patch.

I have applied and tested patch #31 and it solves the problem. I have just started converting the Scheduler module 8.x tests to use BrowserTestBase not WebTestBase. I was getting Call to undefined method Drupal\Tests\scheduler\Functional\SchedulerFunctionalTest::cronRun(). But with the patch this is fixed. I know that the core automated tests are all passing, but it does no harm to have a real user also confirm that this fixes a contrib test too.

The interdiff 25 to 31 looks fine, but someone with more experience than me should re-mark this issue as RTBC.

Incidentally. I had to add both use Drupal\Tests\Traits\Core\CronRunTrait; to the file and use CronRunTrait; to my test class. First I was expecting that BrowserTestBase would have done this for me, and I would not have to make these additions, but I think reading #25-27 above this is what is expected and all is OK.

This looks great to me as well.

RTBC to me. +1.

Also, the Novice part was done, so removing the tag.

Great work, thanks!

Alright.

I did a straight reroll of 31

And also added the change mentioned in #39.

Patch attached.

I'm not a huge fan of adding more and more methods to BTB even via traits. It feels like we're just going to end up back in WebTest land with a method for everything. But since this is blocking #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) and that goal is far more important than my reservations...

Committed b8d54ee and pushed to 8.3.x. Thanks!

Thanks for the work and the commit.
Is this also going to make it into 8.2.x?

@dawehner how right you are... alexpott--

Just wondering if my question from #44 got overlooked:

Is this also going to make it into 8.2.x?

Tasks in general don't go into 8.2.x - but arguably test tasks that are additions should so you can write tests against both 8.2.x and 8.3.x.

Thanks alexpott, yes that is exactly what I was hoping for. I've already had to write a workaround to allow us to convert our WTB tests to BTB and to commit them. This workaround is now not required at Core 8.3 but it still required at 8.2. I (and I assume many likewise) develop and test my contrib code at both 8.2 and 8.3 and both versions are used in our automated testing on D.O. It would help many other contrib test writers to be able to convert to BTB at 8.2

Thanks @alexpott for agreeing that this does need to get into 8.2. Marking as 'patch to be ported' to Core 8.2.x-dev as per comments #48 to #50.

I know that 'patch to be ported' is not exactly right, but it is the closest status for the situation, and leaving it as 'Fixed' will mean it could get forgotten for two weeks and then closed.

This is not going to happen 8.2.x has just entered commit freeze for 8.2.6 and this is not important enough to break that. It is not part of both 8.3.x and 8.4.x so everything is good.

So are you saying that 8.2.6 is the final 8.2 release and that all 8.x core work will only be done at 8.3 (and now 8.4) ? I tried to search for this announcement on D.O. but found nothing.

OK thanks for those links. I can see now that 8.2.6 will be the final 8.2 release.
I've put the issue version back to 8.3.x as that is where it was fixed.