Problem/Motivation

The test is still extending WebTestBase.

https://www.drupal.org/node/3030340

Proposed resolution

Have it extend \Drupal\Tests\BrowserTestBase

Remaining tasks

  1. Provide a patch
  2. Review it
  3. Commit it.

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

RealnameBasicTest was converted to PHPUnit.

CommentFileSizeAuthor
#17 realname-relocate-tests-3115617-17.patch728 bytesmegachriz
#16 realname-relocate-tests-3115617-16.patch208 bytesmegachriz
#13 Convert-RealnameBasicTest-to-PHPUnit-3115617-13.patch209 byteskwfinken
#2 3115617-3.patch8.15 KBmanuel garcia

Issue fork realname-3115617

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Manuel Garcia created an issue. See original summary.

manuel garcia’s picture

manuel garcia commented
Issue summary: View changes
StatusFileSize
new 3115617-3.patch8.15 KB
manuel garcia’s picture

manuel garcia commented
Status: Active » Needs review
manuel garcia’s picture

manuel garcia commented

I've just realized that an older patch already existed on #3031066: Convert automated tests from Simpletest to PHPUnit - I think this one is more complete as it handles the deprecations as well, in any case, should this patch get committed, I think @idbr should get credited as well https://www.drupal.org/u/idebr

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Reviewed & tested by the community

Patch looks good!

philltran made their first commit to this issue’s fork.

philltran opened merge request !2

  • philltran committed b0c5f70 on 8.x-1.x 
    Issue #3115617 by philltran, Manuel Garcia, kim.pepper, idebr: Convert...

philltran credited idebr.

philltran’s picture

philltran commented
Status: Reviewed & tested by the community » Fixed

Thanks Manuel Garcia, kim.pepper and idebr (from earlier issue)

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Fixed » Needs work

@philltran
Thanks for taking up maintainership for this module!
It looks like that the tests are not executed by the testbot yet, probably because the test file is still located at src/Tests/RealnameBasicTest.php. I think that the test file needs to be moved to tests/src/Functional/RealnameBasicTest.php (that's what the patch from #2 did).

philltran’s picture

philltran commented

@MegaChriz thanks for catching this.

kwfinken’s picture

kwfinken commented
Status: Needs work » Needs review
StatusFileSize
new Convert-RealnameBasicTest-to-PHPUnit-3115617-13.patch209 bytes

Quick patch to move the RealnameBasicTest.php file to src/Tests/Functional.

Status: Needs review » Needs work

The last submitted patch, 13: Convert-RealnameBasicTest-to-PHPUnit-3115617-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

philltran’s picture

philltran commented

@kwfinken Thanks for the patch. I also had moved the file on my local but ran into the test failing locally for D9 on the two lines below inside the update user name test.

    // Check if realname changed.
     $this->assertTrue($realname1);
     $this->assertTrue($realname2);

Looks like your test for D 8.9 passed. Maybe it's just my dev environment.

megachriz’s picture

megachriz
Primary language Dutch
commented
StatusFileSize
new realname-relocate-tests-3115617-16.patch208 bytes

@philltran
assertTrue() shouldn't be used for non-boolean values. Based on reading the test, I guess both $realname1 and $realname2 are a string.

2x: Support for asserting against non-boolean values in ::assertTrue is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use a different assert method, for example, ::assertNotEmpty(). See https://www.drupal.org/node/3082086
2x in RealnameBasicTest::testRealnameUserUpdate from Drupal\Tests\realname\Functional

The test is still in the wrong folder. Fixed this in attached patch. Leaving to "Needs work" because there are test failures on D9.

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs work » Needs review
StatusFileSize
new realname-relocate-tests-3115617-17.patch728 bytes

Weird that the patch did not apply. Let's try that again. Now also changed the two assertTrue() assertions with assertNotEmpty().

philltran’s picture

philltran commented
Status: Needs review » Reviewed & tested by the community

@MegaChriz Thanks! I will try to get this committed today.

philltran opened merge request !3

  • philltran committed 1acb92f on 8.x-1.x 
    Issue #3115617 by philltran, MegaChriz, Manuel Garcia, kwfinken, kim....
megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Reviewed & tested by the community » Needs work

It looks like that the latest commit introduced syntax errors:
https://www.drupal.org/pift-ci-job/2049005

MegaChriz opened merge request !4

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs work » Needs review

I have made the following changes in the test class:

  • Fixed the syntax errors;
  • Enabled all assertions in testRealnameUsernameAlter();
  • Removed the return value for setUp() so tests are passing on PHP 7.0 as well. (The return value would probably need to be re-added at some point for Drupal 10 support, but PHP 7.0 is still supported for Drupal 8 right now.)
megachriz’s picture

megachriz
Primary language Dutch
commented

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed

Thanks for approving the merge request! Marking this issue as "Fixed".

Status: Fixed » Closed (fixed)

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