Problem/Motivation

Tests in core/modules/phpass/tests/src/Tests do not run

They need to be in core/modules/phpass/tests/src/Unit and have the Unit namespace in order to run

Steps to reproduce

The following command can be used to determine the tests discovered by phpunit (note that only the functional test is listed and the unit tests are missing):

% ../vendor/bin/phpunit --list-tests | grep '\\phpass'
 - Drupal\Tests\phpass\Functional\GenericTest::testModuleGenericIssues

Proposed resolution

  1. Change the namespace and folder name
  2. Remove outdated testInvalidArguments() test.

Note on the test removal:

Constructor type hints were added in the very last commit of the original MR. Without the type hints it was necessary to cover custom logic leading to the InvalidArgumentException. That was the reason for the test. But now that there is a type hint on the constructor argument and the custom logic is removed, the test isn't necessary anymore (and in fact fails because it expects another exception type). Since tests didn't run either on the original MR, it went undetected that the constructor change failed that particular test.

In order to verify the fix, the following command can be used (note that unit tests are listed in addition to the functional test):

% ../vendor/bin/phpunit --list-tests | grep '\\phpass'
 - Drupal\Tests\phpass\Unit\LegacyPasswordHashingTest::testPasswordNeedsUpdate
 - Drupal\Tests\phpass\Unit\LegacyPasswordHashingTest::testPasswordHashing
 - Drupal\Tests\phpass\Unit\LegacyPasswordHashingTest::testPasswordRehashing
 - Drupal\Tests\phpass\Unit\PasswordVerifyTest::testPasswordHash
 - Drupal\Tests\phpass\Unit\PasswordVerifyTest::testPasswordNeedsRehash
 - Drupal\Tests\phpass\Unit\PasswordVerifyTest::testPasswordCheckUnknownHash
 - Drupal\Tests\phpass\Unit\PasswordVerifyTest::testPasswordCheckSupported
 - Drupal\Tests\phpass\Unit\PasswordVerifyTest::testWithinBounds
 - Drupal\Tests\phpass\Unit\PasswordVerifyTest::testLongPassword"allowed"
 - Drupal\Tests\phpass\Unit\PasswordVerifyTest::testLongPassword"too_long"
 - Drupal\Tests\phpass\Unit\PasswordVerifyTest::testLongPassword"utf8"
 - Drupal\Tests\phpass\Unit\PasswordVerifyTest::testLongPassword"ut8_extended"
 - Drupal\Tests\phpass\Unit\PasswordVerifyTest::testLongPassword"utf8_too_long"
 - Drupal\Tests\phpass\Functional\GenericTest::testModuleGenericIssues

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3393072

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

larowlan created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3258817: Register all of /tests/src/ for class loading

Seems like #3258817: Register all of /tests/src/ for class loading might help prevent this in the future.

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

znerol opened merge request !4983

znerol’s picture

znerol commented

When running locally, the following test fails:

  /**
   * Tests invalid constructor arguments.
   */
  public function testInvalidArguments() {
    $this->expectException(\InvalidArgumentException::class);
    new PhpassHashedPassword('not a number');
  }

I suggest to drop this test entirely. Constructor type hints were added in the very last commit of the original MR. Without the type hints it was necessary to cover custom logic leading to the InvalidArgumentException. That was the reason for the test. But now that there is a type hint on the constructor argument, the test isn't necessary anymore in my opinion.

znerol’s picture

znerol commented
Status: Active » Needs review
znerol’s picture

znerol commented

Apologies, I messed that up completely in the original MR.

smustgrave’s picture

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

Verified this ran at https://drupal-gitlab-job-artifacts.s3.us-west-2.amazonaws.com/85/5c/855...

quietone’s picture

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

I'm triaging RTBC issues. I read the IS and the comments. I noticed that this is also removing a test, which, according to the title is out of scope.

The link in #8 and the IS result in an 'access denied'. That is really unfortunate. Instead of the links we will have to copy/paste output as needed.

@smustgrave, did you review the removed test? Do you agree that it can be removed?

Thinking more about scope, this is actually making two corrections to the original commit. In this case, cleaning up tests. I am generally not a fan of expanding scope but I in this case it makes sense. But then this should have a title change. Maybe this, "Ensure Unit tests in phpass run and remove unneeded LegacyPasswordHashingTest::testInvalidArguments". A bit long but descriptive.

Setting to NR for the above.

znerol’s picture

znerol commented
Issue summary: View changes
znerol’s picture

znerol commented

Updated the issue summary, added instructions on how to detect the missing tests and how to verify the fix.

znerol’s picture

znerol commented
Issue summary: View changes
znerol’s picture

znerol commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Title: Tests in core/modules/phpass/tests/src/Tests do not run » Ensure Unit tests in phpass run and remove unneeded LegacyPasswordHashingTest::testInvalidArguments
Status: Needs review » Reviewed & tested by the community

Also updated title

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 11.x and backported to 10.2.x and 10.1.x

  • larowlan committed 502604fe on 10.1.x 
    Issue #3393072 by znerol: Ensure Unit tests in phpass run and remove...

  • larowlan committed e0fe532d on 10.2.x 
    Issue #3393072 by znerol: Ensure Unit tests in phpass run and remove...

  • larowlan committed 9d4f46c0 on 11.x 
    Issue #3393072 by znerol: Ensure Unit tests in phpass run and remove...

Status: Fixed » Closed (fixed)

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

znerol closed merge request !4983