Skip to content

chore: phpunit 10 support#3242

Open
yenfryherrerafeliz wants to merge 19 commits intoaws:masterfrom
yenfryherrerafeliz:chore_php_unit_10
Open

chore: phpunit 10 support#3242
yenfryherrerafeliz wants to merge 19 commits intoaws:masterfrom
yenfryherrerafeliz:chore_php_unit_10

Conversation

@yenfryherrerafeliz
Copy link
Contributor

Description of changes:

  • Make data provider static functions
  • Use class attributes instead of annotations for @dataProvider and @Covers.
  • Remove/Replace the usage of expectDeprecation, expectDeprecationMessage, expectDeprecationMessageMatches, expectError, expectErrorMessage, expectNotice, expectNoticeMessage, expectWarning, and expectWarningMessage.
  • Migrate phpunit xml config file.
  • Add XDEBUG_MODE set to coverage in the Make file definition step for running tests.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Instead of using expectDeprecation now we use expectException but we do use the set_error_handler
to throw that expected exceptions when expecting warnings or user notices.
Enhance data providers within test cases to avoid the usage of instance members since they become now static functions.
- expectWarning, expectError, expectError, etc. were removed in PHPUnit 10, and a workaround was used in order to replace the usage of those APIs.
The replacement is to use set_error_handler to catch the alert, throw a RuntimeException, and then expect this thrown exception.
- Remove constructor override for TestCase extension.
- Remove CoverClass from methods.
@yenfryherrerafeliz yenfryherrerafeliz marked this pull request as ready for review January 30, 2026 21:25
Copy link
Member

@stobrien89 stobrien89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for the most part, mostly minor things that are easy to miss in such a large PR

)->wait();
$this->assertSame('foo', $c->getAccessKeyId());
$this->assertSame('baz', $c->getSecretKey());
$this->assertNull($c->getSecurityToken());
$this->assertSame($t, $c->getExpiration());
}

/** @doesNotPerformAssertions */
#[CoversNothing]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use #[DoesNotPerformAssertions] for these cases and remove the assertTrue(true)

@@ -260,6 +260,7 @@ public function testPreparesRequestsWithJsonValueTraitString(): void
$this->assertSame('POST', $request->getMethod());
$this->assertSame('http://foo.com/', (string) $request->getUri());
$this->assertSame('', $request->getHeaderLine('Content-Type'));
$this->assertTrue(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be removed. I would audit anywhere we've added this- chances are, they will need to be removed

@@ -182,6 +188,7 @@ public function testPassesComplianceTest(
$this->assertNotTrue($request->hasHeader($header));
}
}
$this->assertTrue(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be removed


/**
* @covers Aws\Api\Shape
* @covers Aws\Api\AbstractModel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to add this coverage back

*/

*/
#[DataProvider('returnsExpiredCredsProvider')]
public function testExtendsExpirationAndSendsRequestIfImdsYieldsExpiredCreds($client)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure the behavior change here short-circuits this test

}, array_keys($s3Operations));

foreach ($commands as $command) {
$command = new Command('CreateBucket', ['Bucket' => 'bucket']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overwrites $command and uses CreateBucket every time


/**
* @covers Aws\S3\S3Client
* @covers Aws\S3\S3ClientTrait
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropped coverage here

@@ -40,6 +41,7 @@ public function tear_down()
{
stream_wrapper_unregister('s3');
$this->client = null;
restore_exception_handler(); // In case it was not restored
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be restore_error_handler()

/**
* @dataProvider retryProvider
*/
#[DataProvider('retryProvider')]
public function testRetriesOnException($success, $writeCalls)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The success test case changed writeCalls from 3 to 1- the retry-then-succeed path is no longer tested. The original test verified the retry mechanism actually works (fail twice, succeed third time).


$this->expectDeprecation();
$this->expectDeprecationMessage('S3 no longer supports MD5 checksums.');
set_error_handler(function ($err, $message) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no restoration of error handler

AWS_ENDPOINT_URL= \
AWS_SUPPRESS_PHP_DEPRECATION_WARNING=true \
vendor/bin/phpunit --testsuite=unit $(TEST)
vendor/bin/phpunit --no-coverage --testsuite=unit $(TEST)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale for skipping coverage here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to use this locally to get faster results when testing. Will revert it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments