Skip to content

PHPLIB-294: Make test suite compatible with PHPUnit 6.4 #489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 12, 2018
Merged

PHPLIB-294: Make test suite compatible with PHPUnit 6.4 #489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"ext-mongodb": "^1.4.0"
},
"require-dev": {
"phpunit/phpunit": "^4.8.36"
"phpunit/phpunit": "^4.8.36 || ^6.4"
},
"autoload": {
"psr-4": { "MongoDB\\": "src/" },
Expand Down
9 changes: 6 additions & 3 deletions tests/GridFS/BucketFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
*/
class BucketFunctionalTest extends FunctionalTestCase
{
/**
* @doesNotPerformAssertions
*/
public function testValidConstructorOptions()
{
new Bucket($this->manager, $this->getDatabaseName(), [
Expand Down Expand Up @@ -138,7 +141,7 @@ public function testDeleteStillRemovesChunksIfFileDoesNotExist($input, $expected
}

/**
* @expectedException PHPUnit_Framework_Error_Warning
* @expectedException PHPUnit\Framework\Error\Warning
*/
public function testDownloadingFileWithMissingChunk()
{
Expand All @@ -150,7 +153,7 @@ public function testDownloadingFileWithMissingChunk()
}

/**
* @expectedException PHPUnit_Framework_Error_Warning
* @expectedException PHPUnit\Framework\Error\Warning
*/
public function testDownloadingFileWithUnexpectedChunkIndex()
{
Expand All @@ -165,7 +168,7 @@ public function testDownloadingFileWithUnexpectedChunkIndex()
}

/**
* @expectedException PHPUnit_Framework_Error_Warning
* @expectedException PHPUnit\Framework\Error\Warning
*/
public function testDownloadingFileWithUnexpectedChunkSize()
{
Expand Down
18 changes: 14 additions & 4 deletions tests/GridFS/ReadableStreamFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,11 @@ public function setUp()
]);
}

public function testValidConstructorFileDocument()
public function testGetFile()
{
new ReadableStream($this->collectionWrapper, (object) ['_id' => null, 'chunkSize' => 1, 'length' => 0]);
$fileDocument = (object) ['_id' => null, 'chunkSize' => 1, 'length' => 0];
$stream = new ReadableStream($this->collectionWrapper, $fileDocument);
$this->assertSame($fileDocument, $stream->getFile());
}

/**
Expand Down Expand Up @@ -108,14 +110,22 @@ public function provideFileIdAndExpectedBytes()
];
}

public function provideFilteredFileIdAndExpectedBytes()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like there is no way to chain data providers so I thought this would be the most straightforward way to provide data to testReadBytesCalledMultipleTimes()

Copy link
Member

Choose a reason for hiding this comment

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

If you want to avoid duplicating the datasets from provideFileIdAndExpectedBytes, you could either call that data provider and run its return value through array_filter. Altternatively, testReadBytesCalledMultipleTimes could just skip all datasets that don't apply with markTestSkipped. Not sure if any of those sound better to you, just providing some ideas.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like there is no way to chain data providers

Were you by chance reading about @depends? That's the only form of test chaining I'm aware of, where the return value of one test can be used as input for a subsequent test case.

Anywho, I think I mentioned using array_filter() earlier this week, which is exactly what @alcaeus is proposing here. Since provideFileIdAndExpectedBytes() returns an array of argument arrays, and we just want to filter out any argument arrays where the length (i.e. second argument) is zero by using an anonymous function as our filter callback:

function(array $args) { return $args[1] > 0; }

Using provideFileIdAndExpectedBytes() as the array_filter() input, each of its 20 argument arrays will be run through our callback and only those where the callback returns true will be kept. This allows us to re-use most of the existing data without duplicating it in the test sources.

{
return array_filter($this->provideFileIdAndExpectedBytes(),
function(array $args) {
return $args[1] > 0;
}
);
}

/**
* @dataProvider provideFileIdAndExpectedBytes
* @dataProvider provideFilteredFileIdAndExpectedBytes
*/
public function testReadBytesCalledMultipleTimes($fileId, $length, $expectedBytes)
{
$fileDocument = $this->collectionWrapper->findFileById($fileId);
$stream = new ReadableStream($this->collectionWrapper, $fileDocument);

for ($i = 0; $i < $length; $i++) {
$expectedByte = isset($expectedBytes[$i]) ? $expectedBytes[$i] : '';
$this->assertSame($expectedByte, $stream->readBytes(1));
Expand Down
2 changes: 1 addition & 1 deletion tests/GridFS/SpecFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ private function getExceptionClassForError($error)
/* Although ReadableStream throws a CorruptFileException, the
* stream wrapper will convert it to a PHP error of type
* E_USER_WARNING. */
return 'PHPUnit_Framework_Error_Warning';
return 'PHPUnit\Framework\Error\Warning';

default:
throw new LogicException('Unsupported error: ' . $error);
Expand Down
3 changes: 3 additions & 0 deletions tests/GridFS/WritableStreamFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ public function setUp()
$this->collectionWrapper = new CollectionWrapper($this->manager, $this->getDatabaseName(), 'fs');
}

/**
* @doesNotPerformAssertions
*/
public function testValidConstructorOptions()
{
new WritableStream($this->collectionWrapper, 'filename', [
Expand Down
1 change: 1 addition & 0 deletions tests/Operation/ReplaceOneTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public function testConstructorReplacementArgumentTypeCheck($replacement)

/**
* @dataProvider provideReplacementDocuments
* @doesNotPerformAssertions
*/
public function testConstructorReplacementArgument($replacement)
{
Expand Down
1 change: 1 addition & 0 deletions tests/Operation/UpdateManyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public function testConstructorUpdateArgumentTypeCheck($update)

/**
* @dataProvider provideUpdateDocuments
* @doesNotPerformAssertions
Copy link
Member

Choose a reason for hiding this comment

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

TIL @doesNotPerformAssertions is a thing.

Perhaps we can use this for the Bucket and WritableStream testValidConstructorOptions() tests?

ReadableStream's testValidConstructorFileDocument() test looks OK with the change you made in this PR (also because no other tests appear to call ReadableStream::getFile()).

*/
public function testConstructorUpdateArgument($update)
{
Expand Down
1 change: 1 addition & 0 deletions tests/Operation/UpdateOneTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public function testConstructorUpdateArgumentTypeCheck($update)

/**
* @dataProvider provideUpdateDocuments
* @doesNotPerformAssertions
*/
public function testConstructorUpdateArgument($update)
{
Expand Down
4 changes: 4 additions & 0 deletions tests/bootstrap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,7 @@
} else {
throw new Exception('Can\'t find autoload.php. Did you install dependencies with Composer?');
}

if ( ! class_exists('PHPUnit\Framework\Error\Warning')) {
class_alias('PHPUnit_Framework_Error_Warning', 'PHPUnit\Framework\Error\Warning');
}
Copy link
Member

Choose a reason for hiding this comment

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

This ensures that any reference to PHPUnit\Framework\Error\Warning will alias to PHPUnit_Framework_Error_Warning, which doesn't currently affect anything. The GridFS tests still use PHPUnit_Framework_Error_Warning, so they work for PHPUnit 4 but fail in 6.

See the renaming I suggested at the bottom of #489 (comment)