Ensure Unit tests in phpass run and remove unneeded LegacyPasswordHashingTest::testInvalidArguments

Seems like 📌 Register all of /tests/src/ for class loading Needs review might help prevent this in the future.

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.

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

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

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.

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

Also updated title

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

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

znerol → closed merge request !4983