Problem/Motivation

The Redirect repository contains a number of test classes that are based on Simpletest but this test framework is deprecated in favor of PHPUnit.

Proposed resolution

Convert the tests extending \Drupal\simpletest\WebTestBase classes to PHPUnit.

Remaining tasks

  1. Write a patch
  2. Review
  3. Commit

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

idebr created an issue. See original summary.

sergiu stici’s picture

Status: Active » Needs review
StatusFileSize
new10.8 KB

Here is the patch, please review.

Status: Needs review » Needs work

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

berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new23.06 KB

WIP patch, does not include #2 as that isn't really what this issue is about, I'll merge that later or move to a different issue.

berdir’s picture

Progress.

Status: Needs review » Needs work

The last submitted patch, 5: redirect-phpunit-3025986-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

berdir’s picture

Status: Needs work » Needs review
StatusFileSize
new42.26 KB

Moving GlobalRedirectTest to a follow-up, that one is tough.

lendude’s picture

  1. +++ b/tests/src/Functional/AssertRedirectTrait.php
    @@ -18,19 +18,30 @@ trait AssertRedirectTrait {
    +    $client = $this->getSession()->getDriver()->getClient()->getClient();
    

    could just use $this->getHttpClient()

  2. +++ b/tests/src/FunctionalJavascript/RedirectJavascriptTest.php
    @@ -0,0 +1,280 @@
    +    // Wait on ajax is unpredictable, wait for one second.
    +    sleep(1);
    

    :( but probably safer to do both? In case it runs longer than 1 second?

  3. +++ b/tests/src/FunctionalJavascript/RedirectJavascriptTest.php
    @@ -0,0 +1,280 @@
    +    // Filter  with non existing value.
    

    2 spaces! :D

And lots of uses of t() that can be removed, but that should probably not be done here

berdir’s picture

1. Didn't know that method, updated.
2. I'm not sure why, but adding the wait on ajax somehow resulted in an exception, that's why I went with 1s... there's some pretty funky ajax going on that form, I'm not sure if something goes wrong if I call wait for ajax and there's actually no ajax call pending: "RuntimeException : Unable to complete AJAX request.". But at the same time, I have to wait, because apparently when I submit when the ajax request is still active, then the value of that form field is dropped o.0.
3. fixed.

lendude’s picture

StatusFileSize
new229.88 KB

As discussed in slack:

The assertWaitOnAjaxRequest() weirdness might be the result from a 403 on the AJAX request in that scenario

berdir’s picture

Ok, that turned out to really be a bug, some paths can throw an access denied exception (not sure why a leading / would cause that, though).

Added a catch for that for now, that makes it work with a regular ajax wait.

berdir’s picture

Status: Needs review » Fixed

Committed.

  • Berdir committed 47026c1 on 8.x-1.x
    Issue #3025986 by Berdir, Sergiu Stici, Lendude: Convert automated tests...
lendude’s picture

Nice, real JS testing that uncovers bugs :)

Status: Fixed » Closed (fixed)

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