Convert \Drupal\system\Tests\Session\SessionHttpsTest to PHPUnit.

This test has some history and deserves its own issue, see #2265099: Cleanup SessionHttpsTest and fix redirect to non-existing URL after POST requests

Scope:
- \Drupal\system\Tests\Session\SessionHttpsTest

CommentFileSizeAuthor
#16 3011079-16.patch21.35 KBLendude
#16 interdiff-3011079-15-16.txt819 bytesLendude
#15 3011079-2-14.patch21.35 KBalexpott
#15 13-14-interdiff.txt1.41 KBalexpott
#13 3011079-2-13.patch21.07 KBalexpott
#12 3011079-2-12.patch21.59 KBalexpott
#8 drupal-session-https-test-to-phpunit-3011079-8.patch3.38 KBscott_euser
#8 interdiff-3011079-6-8.txt678 bytesscott_euser
#6 drupal-session-https-test-to-phpunit-3011079-6.patch3.37 KBscott_euser
#6 interdiff-3011079-2-6.txt1.08 KBscott_euser
#5 drupal-session-https-test-to-phpunit-3011079-5.patch10.27 KBscott_euser
#5 interdiff-3011079-2-5.txt12.24 KBscott_euser
#2 3011079-2.patch2.53 KBLendude
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Lendude created an issue. See original summary.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at Dx Experts commented
Status: Active » Needs review
FileSize
3011079-2.patch2.53 KB

This doesn't work at all, but it might be a first step.

Status: Needs review » Needs work

The last submitted patch, 2: 3011079-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Parent issue: » #2862508: Convert system functional tests to phpunit
scott_euser’s picture

scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commented
Status: Needs work » Needs review
FileSize
interdiff-3011079-2-5.txt12.24 KB
drupal-session-https-test-to-phpunit-3011079-5.patch10.27 KB

Getting this a bit further but I cannot get the mock session value to fill in with my ddev environment setup for tests, so may need someone else to continue on further test fails.

scott_euser’s picture

scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commented
FileSize
interdiff-3011079-2-6.txt1.08 KB
drupal-session-https-test-to-phpunit-3011079-6.patch3.37 KB

Fixed patch to include the removal of the old file; thanks for spotting @dawehner

Setting the interdiff from the original so it's a useful interdiff.

The last submitted patch, 5: drupal-session-https-test-to-phpunit-3011079-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

scott_euser’s picture

scott_euser CreditAttribution: scott_euser as a volunteer and at Soapbox Communications Ltd commented
FileSize
interdiff-3011079-6-8.txt678 bytes
drupal-session-https-test-to-phpunit-3011079-8.patch3.38 KB

Updated with codesniffer results

The last submitted patch, 6: drupal-session-https-test-to-phpunit-3011079-6.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 8: drupal-session-https-test-to-phpunit-3011079-8.patch, failed testing. View results

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented 
+++ b/core/modules/system/tests/src/Functional/Session/SessionHttpsTest.php
@@ -143,12 +156,29 @@ protected function loginHttp(AccountInterface $account) {
+    $html = $this->getSession()->getPage()->getHtml();
...
+    $form_tokens_found = preg_match('/\sname="form_build_id" value="([^"]+)"/', $html, $build_id_match);
+    $this->assertTrue($form_tokens_found, 'Form tokens found in output.');

🔧 How about using something like $this->session->getPage()->findField('form_build_id')

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review
FileSize
3011079-2-12.patch21.59 KB

Here's a passing conversion. We need to disable redirects whilst logging in because otherwise it'll try to redirect us to a https site which might not work. The original test was preventing redirects too but we have to change how we do that because of the Goutte/Mink/BrowserKit architecture. There's also a bit of fun with Guzzle and BrowserKit CookieJar's which are different. For example, BrowserKit's CookieJar does not allow the secure attribute to be set when the cookies domain is not https so we have to test for that using the CookieJar guzzle fills up.

No interdiff because this is the first fully working conversion and all the changes need review. I've tried to only refactor what is necessary / makes things a bit easier. It would be possible to refactor loginHttps() and loginHttp() to share more code and thereby make maintaining this in the future a bit simpler but I don't think we should do that here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
12-13-interdiff.txt4.18 KB
3011079-2-13.patch21.07 KB

Ahh the docs are now out-of-date. Update them.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext commented

#12 looks good. Yeah, I agree code is not DRY but the conversion seems alright.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
13-14-interdiff.txt1.41 KB
3011079-2-14.patch21.35 KB

I don;'t understand having the concurrent secure sessions unless we make some assertions so I've added some. This is a little bit of scope creep I guess but there must have been a reason for adding this and I want to make sure that the assumptions the test is making are still correct so I've added some assertions against those assumptions.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented
Status: Needs review » Reviewed & tested by the community
FileSize
interdiff-3011079-15-16.txt819 bytes
3011079-16.patch21.35 KB

@alexpott nice!

+++ b/core/modules/system/tests/src/Functional/Session/SessionHttpsTest.php
@@ -0,0 +1,310 @@
+use Drupal\Tests\BrowserTestBase;
+use GuzzleHttp\Cookie\CookieJar;
+use Psr\Http\Message\ResponseInterface;
+use Symfony\Component\BrowserKit\Cookie;
+use Symfony\Component\HttpFoundation\Request;
+use Drupal\Component\Utility\Crypt;
+use Drupal\Core\Session\AccountInterface;

The only thing I can find is that these are not in alphabetical order, so quick cleanup for that, but since that is just a cosmetic change I'm going to RTBC this too.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed a51a8f5 and pushed to 8.7.x. Thanks!

  • catch committed a51a8f5 on 8.7.x 
    Issue #3011079 by scott_euser, alexpott, Lendude: Convert \Drupal\system...

Status: Fixed » Closed (fixed)

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