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

kvwalker · 2018-02-07T20:55:32Z

https://jira.mongodb.org/browse/PHPLIB-294

kvwalker · 2018-02-07T20:56:35Z

tests/GridFS/ReadableStreamFunctionalTest.php

@@ -108,14 +110,34 @@ public function provideFileIdAndExpectedBytes()
        ];
    }

+    public function provideFilteredFileIdAndExpectedBytes()


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()

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.

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.

jmikola · 2018-02-08T15:05:21Z

tests/Operation/UpdateManyTest.php

@@ -27,6 +27,7 @@ public function testConstructorUpdateArgumentTypeCheck($update)

    /**
     * @dataProvider provideUpdateDocuments
+     * @doesNotPerformAssertions


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()).

jmikola · 2018-02-08T15:09:22Z

composer.json

@@ -16,7 +16,7 @@
        "ext-mongodb": "^1.4.0"
    },
    "require-dev": {
-        "phpunit/phpunit": "^4.8.36"
+        "phpunit/phpunit": "^6.4"


This causes the 5.x jobs on Travis to fail, since there is no longer a compatible version of PHPUnit:

Problem 1 - phpunit/phpunit 6.5.6 requires php ^7.0 -> your PHP version (5.6.32) does not satisfy that requirement. - phpunit/phpunit 6.5.5 requires php ^7.0 -> your PHP version (5.6.32) does not satisfy that requirement. - phpunit/phpunit 6.5.4 requires php ^7.0 -> your PHP version (5.6.32) does not satisfy that requirement. - phpunit/phpunit 6.5.3 requires php ^7.0 -> your PHP version (5.6.32) does not satisfy that requirement. - phpunit/phpunit 6.5.2 requires php ^7.0 -> your PHP version (5.6.32) does not satisfy that requirement. - phpunit/phpunit 6.5.1 requires php ^7.0 -> your PHP version (5.6.32) does not satisfy that requirement. - phpunit/phpunit 6.5.0 requires php ^7.0 -> your PHP version (5.6.32) does not satisfy that requirement. - phpunit/phpunit 6.4.4 requires php ^7.0 -> your PHP version (5.6.32) does not satisfy that requirement. - phpunit/phpunit 6.4.3 requires php ^7.0 -> your PHP version (5.6.32) does not satisfy that requirement. - phpunit/phpunit 6.4.2 requires php ^7.0 -> your PHP version (5.6.32) does not satisfy that requirement. - phpunit/phpunit 6.4.1 requires php ^7.0 -> your PHP version (5.6.32) does not satisfy that requirement. - phpunit/phpunit 6.4.0 requires php ^7.0 -> your PHP version (5.6.32) does not satisfy that requirement. - Installation request for phpunit/phpunit ^6.4 -> satisfiable by phpunit/phpunit[6.4.0, 6.4.1, 6.4.2, 6.4.3, 6.4.4, 6.5.0, 6.5.1, 6.5.2, 6.5.3, 6.5.4, 6.5.5, 6.5.6].

I'd suggest using "^4.8.36 || ^6.4", which should support PHP 5.x while still allowing PHP 7 to use PHPUnit 6. That syntax is discussed in Version Range in the Composer docs.

jmikola · 2018-02-08T15:35:33Z

tests/bootstrap.php

@@ -9,3 +9,5 @@
 } else {
    throw new Exception('Can\'t find autoload.php. Did you install dependencies with Composer?');
 }
+
+class_alias("PHPUnit\Framework\Error\Warning", "PHPUnit_Framework_Error_Warning");


This is going to fail on PHPUnit 4 (PHP 5.x), as the original class (first argument) doesn't exist. You'll also get a warning because class_alias() will try to autoload that class and fail to find it (controlled by its third boolean argument).

Instead, I'd suggest using the PHPUnit 6 class names in our tests and only creating the alias when needed for compatibility with PHPUnit 4:

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

This condition should only be hit by PHPUnit 4, when the new class isn't found (even after an autoloading attempt). It will then create the PHPUnit\Framework\Error\Warning alias for the old PHPUnit_Framework_Error_Warning class, which will also be autoloaded by default.

AFAIK, the only references to "PHPUnit_Framework_Error_Warning" are in:

tests/GridFS/BucketFunctionalTest.php

tests/GridFS/SpecFunctionalTest.php

Most are in @expectedException annotations but at least one is a string.

jmikola · 2018-02-08T15:45:31Z

tests/GridFS/ReadableStreamFunctionalTest.php

@@ -108,14 +110,34 @@ public function provideFileIdAndExpectedBytes()
        ];
    }

+    public function provideFilteredFileIdAndExpectedBytes()


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.

jmikola · 2018-02-08T15:49:50Z

We're skipping PHPUnit 5 entirely and are keeping PHPUnit 4 compat, so I might also rephrase the commit and PR as:

Make test suite compatible with PHPUnit 6.4

jmikola · 2018-02-08T17:55:31Z

tests/GridFS/BucketFunctionalTest.php

@@ -17,6 +17,9 @@
 */
 class BucketFunctionalTest extends FunctionalTestCase
 {
+    /*


This is missing a second * in order to be considered a doc block. Without it, ReflectionFunction::getDocComment() is not able to pick it up for parsing by PHPUnit.

jmikola · 2018-02-08T17:56:29Z

tests/GridFS/ReadableStreamFunctionalTest.php

+                return $args[1] > 0;
+            }
+        );
+        return $filteredFiles;


This can be factored out so you simply return array_filter(...);.

jmikola · 2018-02-08T17:56:53Z

tests/GridFS/WritableStreamFunctionalTest.php

@@ -19,6 +19,9 @@ public function setUp()
        $this->collectionWrapper = new CollectionWrapper($this->manager, $this->getDatabaseName(), 'fs');
    }

+    /*


Also needs another *.

jmikola · 2018-02-08T18:00:10Z

tests/bootstrap.php

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


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)

jmikola

Requesting a rename of one test. LGTM otherwise.

jmikola · 2018-02-12T15:25:32Z

tests/GridFS/ReadableStreamFunctionalTest.php

@@ -40,7 +40,9 @@ public function setUp()

    public function testValidConstructorFileDocument()


Last minute request: can we change the test name to testGetFile()?

kvwalker requested a review from jmikola February 7, 2018 20:55

kvwalker commented Feb 7, 2018

View reviewed changes

jmikola requested changes Feb 8, 2018

View reviewed changes

jmikola changed the title ~~PHPLIB-294: Make test suite compatible with PHPUnit 4.8, 5.7, and 6.4~~ PHPLIB-294: Make test suite compatible with PHPUnit 6.4 Feb 8, 2018

kvwalker force-pushed the PHPLIB-294 branch from 3664424 to 278c8e9 Compare February 8, 2018 17:20

jmikola reviewed Feb 8, 2018

View reviewed changes

jmikola approved these changes Feb 12, 2018

View reviewed changes

PHPLIB-294: Make test suite compatible with PHPUnit 6.4

a8b4862

kvwalker force-pushed the PHPLIB-294 branch from f1bc0bb to a8b4862 Compare February 12, 2018 15:35

kvwalker merged commit a8b4862 into mongodb:master Feb 12, 2018

kvwalker added a commit that referenced this pull request Feb 12, 2018

Merge pull request #489

17da06f

kvwalker deleted the PHPLIB-294 branch February 12, 2018 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

kvwalker commented Feb 7, 2018

Uh oh!

kvwalker Feb 7, 2018

Uh oh!

alcaeus Feb 8, 2018

Uh oh!

jmikola Feb 8, 2018

Uh oh!

jmikola Feb 8, 2018

Uh oh!

jmikola Feb 8, 2018

Uh oh!

jmikola Feb 8, 2018

Uh oh!

jmikola Feb 8, 2018

Uh oh!

jmikola commented Feb 8, 2018

Uh oh!

jmikola Feb 8, 2018

Uh oh!

jmikola Feb 8, 2018

Uh oh!

jmikola Feb 8, 2018

Uh oh!

jmikola Feb 8, 2018

Uh oh!

jmikola left a comment

Uh oh!

jmikola Feb 12, 2018

Uh oh!

Uh oh!

		@@ -40,7 +40,9 @@ public function setUp()

		public function testValidConstructorFileDocument()

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

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

Uh oh!

Conversation

kvwalker commented Feb 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!