Problem/Motivation

checkForMetaRefresh() is used in drupalGet() and drupalPostForm() in old Simpletest. We need to do the same for BrowserTestBase so that tests involving Batch API work.

Proposed resolution

Implement that on BrowserTEstBase and convert Batch API tests to prove it works.

Remaining tasks

Patch.

CommentFileSizeAuthor
#16 interdiff-2855942-14-16.txt665 bytesdagmar
#16 2855942-16.patch6.7 KBdagmar
#16 2855942-16-test-only.patch3.24 KBdagmar
#14 2855942-14.patch6.82 KBdagmar
#14 2855942-14-test-only.patch3.37 KBdagmar
#14 interdiff-2855942-12-14.txt461 bytesdagmar
#12 2855942-12.patch6.81 KBdagmar
#12 2855942-12-test-only.patch3.35 KBdagmar
#5 interdiff-2-to-5.txt5.06 KBclaudiu.cristea
#5 2855942-5.patch1.65 KBclaudiu.cristea
#2 2855942-2.patch5.54 KBclaudiu.cristea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Title: Move WebTestBase::checkForMetaRefresh() to be used by BTB & WTB » Move WebTestBase::checkForMetaRefresh() in a trait to be used by BTB & WTB
Status: Active » Needs review
FileSize
2855942-2.patch5.54 KB

Patch.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: 2855942-2.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Title: Move WebTestBase::checkForMetaRefresh() in a trait to be used by BTB & WTB » Create ::checkForMetaRefresh() in a trait to be used by BTB tests
Issue summary: View changes
FileSize
2855942-5.patch1.65 KB
interdiff-2-to-5.txt5.06 KB

No, this issue should redefine its objectives because: The workaround for a trait to serve both, WebTestBase and BrowserTestBase is too ugly and we don't want to push such ugliness in core. My proposal is to create a clean trait ONLY for BrowserTestBase tests and let the WebTestBase as it is. Anyway we'll get rid of it in D9.

The problem now is that we don't have a use-case to test the new trait/method. For this reason I think we should move this trait addition back to #2757023: Convert all aggregator web tests to BrowserTestBase wher we have such a case. Opinions?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Can we somehow provide a test to check that this works? Can we maybe convert one of the tests, like the batch tests to see whether stuff works?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented

@dawehner, quoting myself from #5:

The problem now is that we don't have a use-case to test the new trait/method. For this reason I think we should move this trait addition back to #2757023: Convert all aggregator web tests to BrowserTestBase where we have such a case. Opinions?

TL;DR: Close this in favour of #2757023: Convert all aggregator web tests to BrowserTestBase

GoZ’s picture

GoZ CreditAttribution: GoZ at Barbe-Rousse, Centarro commented
Status: Needs review » Closed (duplicate)

moved to #2757023: Convert all aggregator web tests to BrowserTestBase

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
CreditAttribution: dagmar as a volunteer and at Globant commented
Status: Closed (duplicate) » Needs review

Sorry to reopen this, but I checked #2757023: Convert all aggregator web tests to BrowserTestBase and don't see how this trait is being used.

From #7

Can we somehow provide a test to check that this works? Can we maybe convert one of the tests, like the batch tests to see whether stuff works?

I found a use case here #2863372: Convert dblog web tests to BrowserTestBase where canceling a user account trigger the batch api. As I mentioned in #2863372-7: Convert dblog web tests to BrowserTestBase I'm not sure if the way the trait functionality is used is the right one.

Anyway, I'm reopening this because now we have 3 tests that require this trait, maybe we can use the use case I mentioned before to check if this is working as expected.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

I agree with @dagmar here, we should fix this here as its needed in multiple places.
We should also note that just adding the method will not make drupalGet magically work.

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
CreditAttribution: dagmar as a volunteer and at Globant commented
FileSize
2855942-12-test-only.patch3.35 KB
2855942-12.patch6.81 KB

Here is the conversion of the UserCancelTest because almost doesn't need major changes and basically fails due the lack of batch api support by BTB. It proves the MetaRefreshTrait provides a really useful feature.

Locally I have one fail. I'm not sure if is an error on the batch implementation or in the user module.

Status: Needs review » Needs work

The last submitted patch, 12: 2855942-12.patch, failed testing.

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
CreditAttribution: dagmar as a volunteer and at Globant commented
Status: Needs work » Needs review
FileSize
interdiff-2855942-12-14.txt461 bytes
2855942-14-test-only.patch3.37 KB
2855942-14.patch6.82 KB

Status: Needs review » Needs work

The last submitted patch, 14: 2855942-14.patch, failed testing.

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
CreditAttribution: dagmar as a volunteer and at Globant commented
Status: Needs work » Needs review
FileSize
2855942-16-test-only.patch3.24 KB
2855942-16.patch6.7 KB
interdiff-2855942-14-16.txt665 bytes

For some reason I couldn't determinate the getTextContent() is not returning the content of the admin/people screen. I'm removing the problematic line of code for now, maybe someone else can say what is going on here.

The last submitted patch, 16: 2855942-16-test-only.patch, failed testing.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
Title: Create ::checkForMetaRefresh() in a trait to be used by BTB tests » Create ::checkForMetaRefresh() on BrowserTestBase
Issue summary: View changes
Status: Needs review » Needs work

It looks like we will need checkForMetaRefresh() on BrowserTestBase directly because drupalGet() and drupalPostForm() also use it in old Simpletest. We want Batch API pages to be executed automatically, same as we did in old Simpletest, so I think we should add this directly. Sorry for the confusion.

So I would propose that we convert modules/system/src/Tests/Batch to browser tests with this issue, that will give us the functional test coverage when we add checkForMetaRefresh() within BrowserTestBase. See also #2862885: Batch: Convert system functional tests to phpunit.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

I totally agree. I think we should actually not convert the user tests as well and instead just convert the batch ones.

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
CreditAttribution: dagmar as a volunteer and at Globant commented
Status: Needs work » Closed (duplicate)

The real progress is happening here: #2862885: Batch: Convert system functional tests to phpunit