Problem/Motivation

The batch api cannot be used with phpunit. In old Simpletest checkForMetaRefresh() is used in drupalGet() and drupalPostForm(). We need to do the same for BrowserTestBase so that tests involving Batch API work.

Proposed resolution

Convert modules/system/src/Tests/Batch to browser tests, that will give us the functional test coverage when we add checkForMetaRefresh() within BrowserTestBase.

Remaining tasks

User interface changes

None

API changes

New MetaRefreshTrait trait.

CommentFileSizeAuthor
#55 2862885-55.patch6.11 KBdagmar
#51 2862885-51.patch6.87 KBLendude
#51 interdiff-2862885-49-51.txt513 bytesLendude
#49 2862885-49.patch6.92 KBLendude
#49 interdiff-2862885-44-49.txt3.49 KBLendude
#44 2862885-44-test-only.patch1.46 KBdagmar
#44 interdiff-2862885-41-44.txt1.33 KBdagmar
#44 2862885-44.patch7.24 KBdagmar
#41 2862885-41.patch5.82 KBLendude
#41 interdiff-2862885-39-41.txt1.51 KBLendude
#39 2862885-39.patch5.63 KBdagmar
#37 2862885-37.patch9.67 KBdagmar
#29 interdiff-2862885-25-29.patch692 bytesdagmar
#29 2862885-29.patch17.09 KBdagmar
#25 interdiff-2862885-23-25.txt1.19 KBdagmar
#25 2862885-25.patch17.11 KBdagmar
#23 2862885-23.patch17.09 KBdagmar
#23 interdiff-2862885-18-23.txt3.65 KBdagmar
#18 2862885-18.patch16.22 KBdagmar
#18 interdiff-2862885-13-18.txt1.36 KBdagmar
#16 interdiff-2862885-13-14.txt1.36 KBdagmar
#16 2862885-14.patch15.56 KBdagmar
#13 2862885-13.patch14.2 KBdagmar
#8 interdiff-2862885-6.8.txt11.16 KBmartin107
#8 convert-2862885-8.patch12.3 KBmartin107
#6 2862885-6.patch2.39 KBjofitz
#6 interdiff-2-6.txt1.36 KBjofitz
#2 initial-2862885-2.patch2.44 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107 created an issue. See original summary.

martin107’s picture

Status: Active » Needs review
FileSize
2.44 KB

Initial attempt.

1) Just moving files into the correct directory structure.
2) Taking care of namespaces.
3) maximumMetaRefreshCount is vestigial - set but never used... deleting.

Status: Needs review » Needs work

The last submitted patch, 2: initial-2862885-2.patch, failed testing.

gaurav.kapoor’s picture

Assigned: martin107 » gaurav.kapoor
gaurav.kapoor’s picture

Assigned: gaurav.kapoor » Unassigned
jofitz’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
2.39 KB

Extend BrowserTestBase rather than JavascriptTestBase (although I doubt this will solve the problem).

Status: Needs review » Needs work

The last submitted patch, 6: 2862885-6.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
12.3 KB
11.16 KB

This won't solve the problem either, I am just mechanically turning a handle
and converting a few assertText functions.

agomezmoron’s picture

Status: Needs review » Needs work

Hi @martin107,

So it should be marked as NW again, isn't it?

jofitz’s picture

@martin107 I can highly recommend setting up a local test environment it can help you catch a lot of test failures and avoids too much 'turning the handle'.

martin107’s picture

@agomezmoron,

I was expecting the same amount of fails as the previous patch, so testbot would automatically knock the test back to NW
yes the task needs more work.

@Jo Fitzgerald

My test setup is fully functional, I have a limited amount of each day for open source stuff...so all my stuff is micro-adjustments, it is just the way of things!

dawehner’s picture

Note: #2855942: Create ::checkForMetaRefresh() on BrowserTestBase contains the fixes we need here. I recommend to copy the test conversion from here over to there + use the new trait.

dagmar’s picture

Joining the two patches for now. @dawehner do you think we should include waitBatchProccessToEnd inside drupalPostForm?

At least this is what I understand from @kalusi comment on https://www.drupal.org/node/2855942#comment-12014304

Also if we are going to fix the batch support for all the issues depending on this, we should update the issue summary.

dawehner’s picture

Joining the two patches for now. @dawehner do you think we should include waitBatchProccessToEnd inside drupalPostForm?

I'd say yes as well.

dagmar’s picture

Do we need a trait then?

dagmar’s picture

Status: Needs work » Needs review
FileSize
15.56 KB
1.36 KB

Status: Needs review » Needs work

The last submitted patch, 16: 2862885-14.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
16.22 KB
dagmar’s picture

Status: Needs review » Needs work

The last submitted patch, 18: 2862885-18.patch, failed testing.

michielnugter’s picture

Issue tags: +phpunit initiative
michielnugter’s picture

dagmar’s picture

Status: Needs work » Needs review
FileSize
3.65 KB
17.09 KB

Status: Needs review » Needs work

The last submitted patch, 23: 2862885-23.patch, failed testing.

dagmar’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
17.11 KB
1.19 KB
martin107’s picture

As a review I have a little comment

MetaRefreshTrait::checkForMetaRefresh()

  protected function checkForMetaRefresh() {
    $refresh = $this->cssSelect('meta[http-equiv="Refresh"]');
    usleep(100000);
    if (!empty($refresh) && (!isset($this->maximumMetaRefreshCount) || $this->metaRefreshCount < $this->maximumMetaRefreshCount)) {
      // Parse the content attribute of the meta tag for the format:
      // "[delay]: URL=[page_to_redirect_to]".
      if (preg_match('/\d+;\s*URL=(?<url>.*)/i', $refresh[0]->getAttribute('content'), $match)) {
        $this->metaRefreshCount++;
        return $this->drupalGet($this->getAbsoluteUrl(Html::decodeEntities($match['url'])));
      }
    }
    return FALSE;
  }

I have a small thing to make this more tunable - add a method parameter.

checkForMetaRefresh($delay=100000)

and them inside

usleep($delay);

dagmar’s picture

Actually, I'm not sure if we need the delay for DrupaCI. The thing is without this small delay my environment wasn't able to finish the test. Introducing usleep was the only way to make it work.

dawehner’s picture

I have a small thing to make this more tunable - add a method parameter.

To be honest testing batch request is kind of a rare thing. As long there is no usecase I would avoid some parameter

Actually, I'm not sure if we need the delay for DrupaCI. The thing is without this small delay my environment wasn't able to finish the test. Introducing usleep was the only way to make it work.

Do you understand why? This would be kind of interesting, and its probably needed to get this patch in.

dagmar’s picture

I tried in a different setup (now using DrupalVM) and it worked without the need of usleep. So it seems is a environment related failure. No idea why though. The testcase that fail is ProcessingTest, PageTest always worked.

Anyway. Here is the new patch without the usleep call.

Status: Needs review » Needs work

The last submitted patch, 29: interdiff-2862885-25-29.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review

The last submitted patch, 13: 2862885-13.patch, failed testing.

dagmar’s picture

Priority: Normal » Major
Issue tags: -Needs issue summary update

This is blocking all the BTB conversions that require the use of the batch API. I updated the issue summary in #25

dawehner’s picture

@dagmar
So I guess the usleep was actually not a problemat all? In case you have any knowledge about what could have maybe been the problem, please share it.

dagmar’s picture

@dawehner yes, it seems usleep is not necessary. Honestly I don't have any clue why this was required in my current environment (nginx + php-fpm 7.1). But in drupal vm (Apache + php-fpm 7.1) works.

Maybe my web-server is not configured properly. But batch works from the browser, so I don't know.

dawehner’s picture

Status: Needs review » Needs work

I think you ran into #2851111: fastcgi_finish_request and shutdown functions (e.g. batch) do not play nicely together

+++ b/core/modules/system/tests/src/Functional/Batch/ProcessingTest.php
@@ -46,73 +50,86 @@ public function testBatchRedirectFinishedCallback() {
-    $this->assertNoEscaped('<', 'No escaped markup is present.');
+    $assertSession->assertNoEscaped('<', 'No escaped markup is present.');
     $this->assertBatchMessages($this->_resultMessages('batch_4'), 'Nested batch performed successfully.');
     $this->assertEqual(batch_test_stack(), $this->_resultStack('batch_4'), 'Execution order was correct.');
-    $this->assertText('Redirection successful.', 'Redirection after batch execution is correct.');
+    $assertSession->
+      pageTextContains('Redirection successful.', 'Redirection after batch execution is correct.');
   }
 
...
-    $this->assertText('step 1', 'Form is displayed in step 1.');
+    $assertSession->
+      pageTextContains('step 1', 'Form is displayed in step 1.');
 
...
-    $this->assertText('step 2', 'Form is displayed in step 2.');
-    $this->assertNoEscaped('<', 'No escaped markup is present.');
+    $assertSession->
+      pageTextContains('step 2', 'Form is displayed in step 2.');
+    $assertSession->assertNoEscaped('<', 'No escaped markup is present.');
...
-    $this->assertText('Redirection successful.', 'Redirection after batch execution is correct.');
-    $this->assertNoEscaped('<', 'No escaped markup is present.');
+    $assertSession->
+      pageTextContains('Redirection successful.', 'Redirection after batch execution is correct.');
+    $assertSession->assertNoEscaped('<', 'No escaped markup is present.');
 
...
-    $this->assertText('step 2', 'Form is displayed in step 2.');
+    $assertSession->
+      pageTextContains('step 2', 'Form is displayed in step 2.');

@@ -130,7 +147,8 @@ public function testBatchFormMultipleBatches() {
-    $this->assertText('Redirection successful.', 'Redirection after batch execution is correct.');
+    $this->assertSession()->
+      pageTextContains('Redirection successful.', 'Redirection after batch execution is correct.');
   }
 
   /**
@@ -148,7 +166,8 @@ public function testBatchFormProgrammatic() {

@@ -148,7 +166,8 @@ public function testBatchFormProgrammatic() {
     // The stack contains execution order of batch callbacks and submit
     // handlers and logging of corresponding $form_state->getValues().
     $this->assertEqual(batch_test_stack(), $this->_resultStack('chained', $value), 'Execution order was correct, and $form_state is correctly persisted.');
-    $this->assertText('Got out of a programmatic batched form.', 'Page execution continues normally.');
+    $this->assertSession()->
+      pageTextContains('Got out of a programmatic batched form.', 'Page execution continues normally.');

I don't believe that all those changes are actually needed here. Please keep in mind for the sake of everyone involved (reviewers, committers) minimizing the amount of changes is needed.

dagmar’s picture

Status: Needs work » Needs review
FileSize
9.67 KB

Thanks @dawehner. Indeed! #2851111: fastcgi_finish_request and shutdown functions (e.g. batch) do not play nicely together was my problem. Thanks!

Here is the patch without all the assertions modified.

dawehner’s picture

+++ b/core/modules/system/tests/src/Functional/Batch/PageTest.php
@@ -38,6 +38,7 @@ public function testBatchProgressPageTheme() {
     $this->drupalGet('admin/batch-test/test-theme');
+    $this->waitBatchProccessToEnd();
     // The stack should contain the name of the theme used on the progress

... should we add this to drupalGet as well to minimize the cost of porting tests? Maybe I'm missing some obvious point.

dagmar’s picture

Sounds reasonable. But unfortunately it doesn't work. This patch try to do the same thing that we have in Drupal\simpletest\WebTestBase. Maybe someone inspecting #37 and this patch can find where is the problem.

Status: Needs review » Needs work

The last submitted patch, 39: 2862885-39.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
5.82 KB

The check in submitForm() was in the wrong spot and would only work in JavascriptTestBase. Moved it and added the reset of the counter like it is done in WebTestBase.

dawehner’s picture

The check in submitForm() was in the wrong spot and would only work in JavascriptTestBase. Moved it and added the reset of the counter like it is done in WebTestBase.

Is there a test we can convert to prove this actually works?

dagmar’s picture

core/modules/user/src/Tests/UserCancelTest.php is a good candidate

dagmar’s picture

Here is the same patch also covering the UserCancelTest.

Status: Needs review » Needs work

The last submitted patch, 44: 2862885-44-test-only.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
Lendude’s picture

Status: Needs review » Reviewed & tested by the community

The extra test conversion looks good and gives us coverage that this works for doing the conversions.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I'm not sure that this should be a trait. The idea of traits is that we can start to slim down the complexity of BTB make it less magical. However I think we would expect BTB to be able to test a batch without having to include a trait.

@dawehner @Lendude is there a good reason for this to a trait. If I was writing a test that exercised the batch system I would be surprised that I had to include MetaRefreshTrait. Also it is weird that the trait calls drupalGet().

Lendude’s picture

@alexpott you are right, there is no good reason for it to be a trait. It's way to tightly coupled to BrowserTestBase to be useful outside it, so lets put it in.

larowlan’s picture

+++ b/core/tests/Drupal/Tests/BrowserTestBase.php
@@ -23,15 +23,16 @@
+use Drupal\Tests\Traits\Core\MetaRefreshTrait;

Is this still needed?

Other than that, looks RTBC to me.

Lendude’s picture

Thanks @larowlan for spotting that. And it's gone.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, this looks ready to me

alexpott’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed d45312b and pushed to 8.4.x. Thanks!

This had a merge conflict with 8.3.x - as this is a test only patch I think it is worth back-porting.

Credited myself since my feedback changed the API surface area of the patch and @larowlan for the code review.

  • alexpott committed d45312b on 8.4.x
    Issue #2862885 by dagmar, Lendude, martin107, Jo Fitzgerald, dawehner,...
dagmar’s picture

Status: Patch (to be ported) » Needs review
FileSize
6.11 KB

Here is the backport for 8.3.x.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Backport looks good, just the cleanup of the use statements order that was conflicting, strictly speaking wasn't really a related change anyway, so no loss.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5f16748 and pushed to 8.3.x. Thanks!

Thanks it is great to have a 8.3.x test run just in case something in the new tests was using something only in 8.4.x

  • alexpott committed 5f16748 on 8.3.x
    Issue #2862885 by dagmar, Lendude, martin107, Jo Fitzgerald, dawehner,...
jhedstrom’s picture

Started seeing this failure #2244855: Invalid Nodetype to importsimplexml_import_dom in contrib tests utilizing BrowserTestBase and traced it back to this commit. Digging into this now.

Status: Fixed » Closed (fixed)

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