-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix tests for MongoDB 7.0 #2579
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.
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
Conversation
Codecov ReportPatch and project coverage have no change.
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #2579 +/- ##
=========================================
Coverage 90.57% 90.57%
Complexity 748 748
=========================================
Files 34 34
Lines 1783 1783
=========================================
Hits 1615 1615
Misses 168 168 ☔ View full report in Codecov by Sentry. |
158fefe
to
c4761f1
Compare
@GromNaN what about the docker-composer file? in my case the tests were trying to run before mongoDB is fully initiated, so I made this change: |
Error message have changed Caused by :: Write conflict during plan execution and yielding is disabled. :: Please retry your operation or multi-document transaction.
Thanks for the reminder. I imported your changes. |
The transaction tests require a replicaset to work. We might add this in config in docker-compose in a future PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides the question about mongosh
@@ -58,13 +58,16 @@ jobs: | |||
- name: Create MongoDB Replica Set | |||
run: | | |||
docker run --name mongodb -p 27017:27017 -e MONGO_INITDB_DATABASE=unittest --detach mongo:${{ matrix.mongodb }} mongod --replSet rs --setParameter transactionLifetimeLimitSeconds=5 | |||
until docker exec --tty mongodb mongo 127.0.0.1:27017 --eval "db.runCommand({ ping: 1 })"; do | |||
|
|||
if [ "${{ matrix.mongodb }}" = "4.4" ]; then MONGOSH_BIN="mongo"; else MONGOSH_BIN="mongosh"; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does mongosh
come from? IIRC it's not distributed with the server package, so if we download it separately we should be able to use it with 4.4 as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in the mongodb docker image. We can remove this line after February 2024 when support for 4.4 will end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. Fine with me :)
pecl install mongodb && docker-php-ext-enable mongodb && \ | ||
pecl install xdebug && docker-php-ext-enable xdebug && \ | ||
docker-php-ext-install -j$(nproc) pdo_mysql zip | ||
|
||
COPY --from=composer:${COMPOSER_VERSION} /usr/bin/composer /usr/local/bin/composer | ||
COPY --from=composer:2.5.8 /usr/bin/composer /usr/local/bin/composer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having it at the top of the file made it easier to locate the version for an update, but no objection to inlining it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't work with my setup.
Docker version 24.0.5, build ced0996600
Docker Compose version 2.20.3
colima version 0.5.5
There is a workaroud if I alias the composer image.
docker/for-mac#2155 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, no worries then 👍
Error message have changed. We don't need to assert on the error message as we already assert there is a
BulkWriteException
.