Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#55 | 2862885-55.patch | 6.11 KB | dagmar |
#51 | 2862885-51.patch | 6.87 KB | Lendude |
#51 | interdiff-2862885-49-51.txt | 513 bytes | Lendude |
#49 | 2862885-49.patch | 6.92 KB | Lendude |
#49 | interdiff-2862885-44-49.txt | 3.49 KB | Lendude |
Comments
Comment #2
martin107 CreditAttribution: martin107 as a volunteer commentedInitial attempt.
1) Just moving files into the correct directory structure.
2) Taking care of namespaces.
3) maximumMetaRefreshCount is vestigial - set but never used... deleting.
Comment #4
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #5
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #6
jofitz CreditAttribution: jofitz at ComputerMinds commentedExtend BrowserTestBase rather than JavascriptTestBase (although I doubt this will solve the problem).
Comment #8
martin107 CreditAttribution: martin107 as a volunteer commentedThis won't solve the problem either, I am just mechanically turning a handle
and converting a few assertText functions.
Comment #9
agomezmoron CreditAttribution: agomezmoron at La Drupalera by Emergya commentedHi @martin107,
So it should be marked as NW again, isn't it?
Comment #10
jofitz CreditAttribution: jofitz at ComputerMinds commented@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'.
Comment #11
martin107 CreditAttribution: martin107 as a volunteer commented@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!
Comment #12
dawehnerNote: #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.
Comment #13
dagmarJoining 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.
Comment #14
dawehnerI'd say yes as well.
Comment #15
dagmarDo we need a trait then?
Comment #16
dagmarComment #18
dagmarComment #19
dagmarComment #21
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #22
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #23
dagmarComment #25
dagmarComment #26
martin107 CreditAttribution: martin107 as a volunteer commentedAs a review I have a little comment
MetaRefreshTrait::checkForMetaRefresh()
I have a small thing to make this more tunable - add a method parameter.
checkForMetaRefresh($delay=100000)
and them inside
usleep($delay);
Comment #27
dagmarActually, 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.
Comment #28
dawehnerTo be honest testing batch request is kind of a rare thing. As long there is no usecase I would avoid some parameter
Do you understand why? This would be kind of interesting, and its probably needed to get this patch in.
Comment #29
dagmarI 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 isProcessingTest
,PageTest
always worked.Anyway. Here is the new patch without the
usleep
call.Comment #31
dagmarComment #33
dagmarThis is blocking all the BTB conversions that require the use of the batch API. I updated the issue summary in #25
Comment #34
dawehner@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.
Comment #35
dagmar@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.
Comment #36
dawehnerI think you ran into #2851111: fastcgi_finish_request and shutdown functions (e.g. batch) do not play nicely together
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.
Comment #37
dagmarThanks @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.
Comment #38
dawehner... should we add this to drupalGet as well to minimize the cost of porting tests? Maybe I'm missing some obvious point.
Comment #39
dagmarSounds 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.
Comment #41
LendudeThe 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.
Comment #42
dawehnerIs there a test we can convert to prove this actually works?
Comment #43
dagmarcore/modules/user/src/Tests/UserCancelTest.php
is a good candidateComment #44
dagmarHere is the same patch also covering the
UserCancelTest
.Comment #46
dagmarComment #47
LendudeThe extra test conversion looks good and gives us coverage that this works for doing the conversions.
Comment #48
alexpottI'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().
Comment #49
Lendude@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.
Comment #50
larowlanIs this still needed?
Other than that, looks RTBC to me.
Comment #51
LendudeThanks @larowlan for spotting that. And it's gone.
Comment #52
larowlanThanks, this looks ready to me
Comment #53
alexpottCommitted 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.
Comment #55
dagmarHere is the backport for 8.3.x.
Comment #56
LendudeBackport 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.
Comment #57
alexpottCommitted 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
Comment #59
jhedstromStarted 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.