Skip to content

Performance: Keep collections and indexes between GridFS tests #1421

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 7 commits into from
Sep 18, 2024
Merged

Performance: Keep collections and indexes between GridFS tests #1421

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1d831e2
Split GridFS bucket tests to improve performance of unit tests
GromNaN Sep 18, 2024
71c286e
Don't recreate collections and indexes on each test for performance
GromNaN Sep 18, 2024
3c5819a
Revert "Split GridFS bucket tests to improve performance of unit tests"
GromNaN Sep 18, 2024
e318798
Cleaning is done on tearDown only
GromNaN Sep 18, 2024
0365984
Remove collections before and after class, to ensure we always have t…
GromNaN Sep 18, 2024
3f65868
Add comments and assertions to testExistingIndexIsReused
GromNaN Sep 18, 2024
71c313e
Fix drop collections when it doesn't exist
GromNaN Sep 18, 2024
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
4 changes: 4 additions & 0 deletions tests/GridFS/BucketFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,10 @@ public function testUploadingFirstFileCreatesIndexes(): void

public function testExistingIndexIsReused(): void
{
// 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');
Comment on lines +819 to +820
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment and assertions on index existence.

Copy link
Member

Choose a reason for hiding this comment

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

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.

$this->filesCollection->createIndex(['filename' => 1.0, 'uploadDate' => 1], ['name' => 'test']);
$this->chunksCollection->createIndex(['files_id' => 1.0, 'n' => 1], ['name' => 'test', 'unique' => true]);

Expand Down
23 changes: 20 additions & 3 deletions tests/GridFS/FunctionalTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace MongoDB\Tests\GridFS;

use MongoDB\Collection;
use MongoDB\Driver\Command;
use MongoDB\GridFS\Bucket;
use MongoDB\Tests\FunctionalTestCase as BaseFunctionalTestCase;

Expand All @@ -28,10 +29,26 @@ public function setUp(): void
parent::setUp();

$this->bucket = new Bucket($this->manager, $this->getDatabaseName());
$this->bucket->drop();

$this->chunksCollection = $this->createCollection($this->getDatabaseName(), 'fs.chunks');
$this->filesCollection = $this->createCollection($this->getDatabaseName(), 'fs.files');
$this->chunksCollection = new Collection($this->manager, $this->getDatabaseName(), 'fs.chunks');
$this->filesCollection = new Collection($this->manager, $this->getDatabaseName(), 'fs.files');
}

public function tearDown(): void
{
$this->chunksCollection->deleteMany([]);
$this->filesCollection->deleteMany([]);

parent::tearDown();
}

public static function tearDownAfterClass(): void
{
$manager = static::createTestManager();
$manager->executeCommand(self::getDatabaseName(), new Command(['drop' => 'fs.chunks']));
$manager->executeCommand(self::getDatabaseName(), new Command(['drop' => 'fs.files']));

parent::tearDownAfterClass();
}

/**
Expand Down