Performance: Keep collections and indexes between GridFS tests #1421

GromNaN · 2024-09-18T11:45:22Z

Performance impact on my Mac M1 Pro, PHP 8.3, local replica set on MongoDB 8.

Before:

Time: 01:42.729, Memory: 36.00 MB

OK (319 tests, 3164 assertions)

After:

Time: 00:17.596, Memory: 36.00 MB

OK (319 tests, 3164 assertions)

This reverts commit 1d831e2.

GromNaN · 2024-09-18T15:07:27Z

tests/GridFS/BucketFunctionalTest.php

+        $this->dropCollection($this->getDatabaseName(), 'fs.chunks');
+        $this->dropCollection($this->getDatabaseName(), 'fs.files');


By calling dropCollection, the collection is also removed on tearDown.

My suggestion to explain the optimization in a comment within tearDown() would pair nicely with this comment.

jmikola · 2024-09-18T16:09:00Z

tests/GridFS/BucketFunctionalTest.php

+        // The collections may exist from other tests, ensure they are removed before and after the test
+        $this->dropCollection($this->getDatabaseName(), 'fs.chunks');
+        $this->dropCollection($this->getDatabaseName(), 'fs.files');
+


I'm curious about the mixed use of 1.0 and 1 when creating the indexes below.

@alcaeus: it looks like you added this in 79c2993 for PHPLIB-522 ("loosening index equality checks"). Assuming this is still relevant, is there a short comment we can add here explaining what's being tested? I don't think testExistingIndexIsReused() really captures what's going on here.

Maybe a multi-line comment:

Create indexes with different numeric types before interacting with GridFS to assert that PHPLIB respects the existing indexes and does not attempt to create its own.

I'm also curious if there should have been assertIndexExists() calls for the two indexes we did create, although I assume we're relying on a successful command response from the server.

Note: this is admittedly unrelated from this PR, so feel free to disregard. I can also address this in a separate PR myself, but wanted to ask for some context first.

I added a comment and assertions on index existence.

Oof, trying to think back to it. I believe the issue was that the shell used slightly different index settings, resulting in valid indexes that weren't recognised. It may have been an issue with using a double for the index direction instead of an int. I don't remember precisely though, indicating that I should've left a comment at the time.

tests/GridFS/FunctionalTestCase.php

jmikola · 2024-09-18T16:16:09Z

tests/GridFS/BucketFunctionalTest.php

+        $this->dropCollection($this->getDatabaseName(), 'fs.chunks');
+        $this->dropCollection($this->getDatabaseName(), 'fs.files');


My suggestion to explain the optimization in a comment within tearDown() would pair nicely with this comment.

…he same clean setup

* v1.x: (87 commits) Performance: Keep collections and indexes between GridFS tests (#1421) Add final annotations to non-internal Operation classes (#1410) Fix types accepted by $round (#1401) Replace arrayHasKey with assertArrayHasKey in tests (#1403) PHPLIB-1514 Make data providers static (#1404) PHPLIB-1515 Replace assertObjectHasAttribute with assertObjectHasProperty (#1405) Restore Prose22_RangeExplicitEncryptionTest (#1400) Remove Prose22_RangeExplicitEncryptionTest that requires ext-mongodb < 1.20 (#1394) Use `match` instead of `switch` when a simple value is returned (#1393) Remove PHPUnit functions polyfill (#1395) Update branch names for GHA workflows (#1390) PHPLIB-1419 Encode Agg builder objects in Collection methods (#1383) PHPLIB-1420 Integrate query builder for non-aggregation APIs (#1385) Fix optional parameter declared before required parameter (#1384) PHPLIB-1505 Add driver option "builderEncoder" (#1382) Exclude rector.php from the artifact Check generated files are up-to-date Replace composer package mongodb/builder Skip Pedentry method sort for generated files Remove composer constraints already imposed by the main package ...

* v1.x: (90 commits) Merge v1.20 into v1.x (#1447) PHPLIB-1525 Removes dependency to Symfony PHPUnit bridge (#1413) Change deprecated assertObjectHasAttribute to assertObjectHasProperty (#1432) Performance: Keep collections and indexes between GridFS tests (#1421) Add final annotations to non-internal Operation classes (#1410) Fix types accepted by $round (#1401) Replace arrayHasKey with assertArrayHasKey in tests (#1403) PHPLIB-1514 Make data providers static (#1404) PHPLIB-1515 Replace assertObjectHasAttribute with assertObjectHasProperty (#1405) Restore Prose22_RangeExplicitEncryptionTest (#1400) Remove Prose22_RangeExplicitEncryptionTest that requires ext-mongodb < 1.20 (#1394) Use `match` instead of `switch` when a simple value is returned (#1393) Remove PHPUnit functions polyfill (#1395) Update branch names for GHA workflows (#1390) PHPLIB-1419 Encode Agg builder objects in Collection methods (#1383) PHPLIB-1420 Integrate query builder for non-aggregation APIs (#1385) Fix optional parameter declared before required parameter (#1384) PHPLIB-1505 Add driver option "builderEncoder" (#1382) Exclude rector.php from the artifact Check generated files are up-to-date ...

* v1.x: (95 commits) PHPLIB-1369 Upgrade to PHPUnit 10 (#1412) Higher phpunit version required (#1463) Fix deprecations in tests (#1458) Deprecate functionality to be removed (#1441) Expect BulkWriteException (#1455) Merge v1.20 into v1.x (#1447) PHPLIB-1525 Removes dependency to Symfony PHPUnit bridge (#1413) Change deprecated assertObjectHasAttribute to assertObjectHasProperty (#1432) Performance: Keep collections and indexes between GridFS tests (#1421) Add final annotations to non-internal Operation classes (#1410) Fix types accepted by $round (#1401) Replace arrayHasKey with assertArrayHasKey in tests (#1403) PHPLIB-1514 Make data providers static (#1404) PHPLIB-1515 Replace assertObjectHasAttribute with assertObjectHasProperty (#1405) Restore Prose22_RangeExplicitEncryptionTest (#1400) Remove Prose22_RangeExplicitEncryptionTest that requires ext-mongodb < 1.20 (#1394) Use `match` instead of `switch` when a simple value is returned (#1393) Remove PHPUnit functions polyfill (#1395) Update branch names for GHA workflows (#1390) PHPLIB-1419 Encode Agg builder objects in Collection methods (#1383) ...

* v1.x: (101 commits) PHPLIB-1541: Include specs repository as a submodule (#1429) PHPLIB-1548 Inherit `typeMap` option in `Collection::listSearchIndexes()` (#1482) Fix array shape for `Collection::listSearchIndex($options)` (#1480) PHPLIB-1545: Deprecate CreateCollection flags option and related constants (#1477) Fix junit logging (#1475) Deprecate typeMap on operations without meaningful result (#1473) PHPLIB-1369 Upgrade to PHPUnit 10 (#1412) Higher phpunit version required (#1463) Fix deprecations in tests (#1458) Deprecate functionality to be removed (#1441) Expect BulkWriteException (#1455) Merge v1.20 into v1.x (#1447) PHPLIB-1525 Removes dependency to Symfony PHPUnit bridge (#1413) Change deprecated assertObjectHasAttribute to assertObjectHasProperty (#1432) Performance: Keep collections and indexes between GridFS tests (#1421) Add final annotations to non-internal Operation classes (#1410) Fix types accepted by $round (#1401) Replace arrayHasKey with assertArrayHasKey in tests (#1403) PHPLIB-1514 Make data providers static (#1404) PHPLIB-1515 Replace assertObjectHasAttribute with assertObjectHasProperty (#1405) ...

GromNaN force-pushed the tests-gridfs-perfs branch from c930a27 to 4689540 Compare September 18, 2024 11:45

GromNaN added 3 commits September 18, 2024 16:53

Split GridFS bucket tests to improve performance of unit tests

1d831e2

Don't recreate collections and indexes on each test for performance

71c286e

Revert "Split GridFS bucket tests to improve performance of unit tests"

3c5819a

This reverts commit 1d831e2.

GromNaN force-pushed the tests-gridfs-perfs branch from 3a2f35d to 3c5819a Compare September 18, 2024 14:55

GromNaN changed the title ~~Split GridFS bucket tests to improve performance of unit tests~~ Keep collections and indexes between GridFS tests Sep 18, 2024

GromNaN marked this pull request as ready for review September 18, 2024 15:01

GromNaN requested a review from a team as a code owner September 18, 2024 15:01

GromNaN requested review from alcaeus and jmikola and removed request for alcaeus September 18, 2024 15:01

Cleaning is done on tearDown only

e318798

GromNaN commented Sep 18, 2024

View reviewed changes

GromNaN changed the title ~~Keep collections and indexes between GridFS tests~~ Performance: Keep collections and indexes between GridFS tests Sep 18, 2024

jmikola reviewed Sep 18, 2024

View reviewed changes

GromNaN added 3 commits September 18, 2024 18:31

Remove collections before and after class, to ensure we always have t…

0365984

…he same clean setup

Add comments and assertions to testExistingIndexIsReused

3f65868

Fix drop collections when it doesn't exist

71c313e

jmikola approved these changes Sep 18, 2024

View reviewed changes

GromNaN merged commit b27a61d into mongodb:v1.x Sep 18, 2024
31 checks passed

GromNaN deleted the tests-gridfs-perfs branch September 18, 2024 20:45

mongodb-php-bot mentioned this pull request Sep 18, 2024

Merge v1.x into v2.x #1422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance: Keep collections and indexes between GridFS tests #1421

Performance: Keep collections and indexes between GridFS tests #1421

GromNaN commented Sep 18, 2024 •

edited

Loading

GromNaN Sep 18, 2024

jmikola Sep 18, 2024

jmikola Sep 18, 2024

jmikola Sep 18, 2024

GromNaN Sep 18, 2024

alcaeus Sep 19, 2024

jmikola Sep 18, 2024

		$this->dropCollection($this->getDatabaseName(), 'fs.chunks');
		$this->dropCollection($this->getDatabaseName(), 'fs.files');

Performance: Keep collections and indexes between GridFS tests #1421

Performance: Keep collections and indexes between GridFS tests #1421

Conversation

GromNaN commented Sep 18, 2024 • edited Loading

GromNaN Sep 18, 2024

Choose a reason for hiding this comment

jmikola Sep 18, 2024

Choose a reason for hiding this comment

jmikola Sep 18, 2024

Choose a reason for hiding this comment

jmikola Sep 18, 2024

Choose a reason for hiding this comment

GromNaN Sep 18, 2024

Choose a reason for hiding this comment

alcaeus Sep 19, 2024

Choose a reason for hiding this comment

jmikola Sep 18, 2024

Choose a reason for hiding this comment

GromNaN commented Sep 18, 2024 •

edited

Loading