Skip to content

Conversation

@GromNaN
Copy link
Member

@GromNaN GromNaN commented Sep 23, 2025

Q A
Type improvement
BC Break no
Fixed issues PHPORM-390

Summary

  • Run Atlas tests with MongoDB Atlas Local
  • Add integration test for Vector Search
  • Add integration test for Search

The configureFailPoint command is not available in Atlas Local. I mark the tests as skipped instead of adding a group to exclude them from atlas.

@GromNaN GromNaN added this to the 2.13.0 milestone Sep 23, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds Atlas Search feature testing using MongoDB Atlas Local Docker, enabling automated testing of vector search and text search functionality that previously required a full Atlas cluster.

  • Implements Atlas Local testing infrastructure for search features
  • Adds comprehensive vector search and text search integration tests
  • Refactors fail point testing to use a shared helper method

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/Doctrine/ODM/MongoDB/Tests/Functional/VectorSearchTest.php New test suite for Atlas Vector Search functionality
tests/Doctrine/ODM/MongoDB/Tests/Functional/AtlasSearchTest.php New test suite for Atlas text search features
tests/Doctrine/ODM/MongoDB/Tests/BaseTestCase.php Centralizes fail point testing with shared helper method
lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php Fixes vector similarity constant value
.github/workflows/atlas-ci.yml New CI workflow for Atlas Local testing
docker-compose.yaml Docker configuration for Atlas Local container
.github/workflows/continuous-integration.yml Excludes atlas tests from regular CI

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

public const VECTOR_SIMILARITY_EUCLIDEAN = 'euclidean';
public const VECTOR_SIMILARITY_COSINE = 'cosine';
public const VECTOR_SIMILARITY_DOT_PRODUCT = 'dot_product';
public const VECTOR_SIMILARITY_DOT_PRODUCT = 'dotProduct';
Copy link

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

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

The constant value was changed from 'dot_product' to 'dotProduct'. This could be a breaking change if existing code relies on the snake_case format. Please verify this matches the expected MongoDB Atlas format and ensure backward compatibility is maintained.

Suggested change
public const VECTOR_SIMILARITY_DOT_PRODUCT = 'dotProduct';
public const VECTOR_SIMILARITY_DOT_PRODUCT = 'dotProduct';
public const VECTOR_SIMILARITY_DOT_PRODUCT_SNAKE_CASE = 'dot_product';

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

@GromNaN was this not actually a BC break because the constant was introduced in 9a0e994. Here, it looks like you're making the value consistent with what you already had in the documentation from the earlier commit.

Feel free to respond and resolve. I just wanted to confirm.

Copy link
Member Author

Choose a reason for hiding this comment

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

This code is not released yet. This is a bug fix as dotProduct is the value expected by MongoDB: https://www.mongodb.com/docs/php-library/current/indexes/atlas-search-index/#create-a-search-index

Comment on lines +50 to +51
// Wait for the search index to be ready (Atlas Local needs time to build the index)
sleep(2);
Copy link
Member Author

Choose a reason for hiding this comment

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

This could be smarter with a method that detect when the indexes are ready: #2830

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

LGTM. I just have some questions but I don't they will warrant further changes.

public const VECTOR_SIMILARITY_EUCLIDEAN = 'euclidean';
public const VECTOR_SIMILARITY_COSINE = 'cosine';
public const VECTOR_SIMILARITY_DOT_PRODUCT = 'dot_product';
public const VECTOR_SIMILARITY_DOT_PRODUCT = 'dotProduct';
Copy link
Member

Choose a reason for hiding this comment

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

@GromNaN was this not actually a BC break because the constant was introduced in 9a0e994. Here, it looks like you're making the value consistent with what you already had in the documentation from the earlier commit.

Feel free to respond and resolve. I just wanted to confirm.


- name: "Run PHPUnit"
run: "vendor/bin/phpunit"
run: "vendor/bin/phpunit --exclude-group=atlas"
Copy link
Member

Choose a reason for hiding this comment

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

Noted that this will skip the tests with sleep(2).

composer-options: "--prefer-dist"

- name: "Run PHPUnit with Atlas Local"
run: "vendor/bin/phpunit"
Copy link
Member

Choose a reason for hiding this comment

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

Noted that this will run the entire test suite with Atlas Local. How do you intend to trigger the atlas-ci.yml jobs? Presumably we don't need this entire matrix run on each PR. Would it make more sense to initially restrict test execution to the "atlas" group to compensate for the tests that will be exluded by continuous-integration.yml?

Copy link
Member Author

Choose a reason for hiding this comment

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

My goal was to run all the tests with Atlas, but that's not possible due to the lack of support of failPoints. So I simplified the test matrix for Atlas.

return $manager->selectServer()->getType() !== Server::TYPE_STANDALONE;
}

protected function createFailPoint(string $failCommand, bool $transient = false, int $times = 1): void
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it was adapted from what we have in PHPLIB, although this method is only useful for the failCommand fail point. That assumption also exists in the tearDown() method.

Just wanted to confirm that this is perfectly suitable for ODM's present needs. In the future, you may want to make this more consistent with PHPLIB's implementation, though.

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 forgot about the PHPLIB implementation, this is definitely a simpler approach as we only need to add 1 fail point at a time.

$this->uow->persist($friendUser);

$this->createFatalFailPoint('insert');
$this->createFailPoint('insert');
Copy link
Member

Choose a reason for hiding this comment

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

Noted that createFatalFailPoint and createTransientFailPoint only differ by way of the "TransientTransactionError" error label.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's why I added the $transient parameter.

@GromNaN GromNaN merged commit 3dfba27 into doctrine:2.13.x Sep 25, 2025
22 of 23 checks passed
@GromNaN GromNaN deleted the PHPORM-390 branch September 25, 2025 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants