Skip to content
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

upgrade to mongodb-memory-server 9.0.0 #13764

Open
wants to merge 10 commits into
base: 7.x
Choose a base branch
from
Open

Conversation

hasezoey
Copy link
Collaborator

Summary

This PR upgrades mongodb-memory-server to 9.0.0(-beta.2), but also does:

  • upgrade all used mongodb version's patch version
  • add mongodb 7.0.0 as a test (because mms more probably supports it with 9.0.0)
  • upgrade deno to 1.36 (because with the new mms version, 1.34 just gets stuck before even starting the server)

Currently known problems:
deno really does not like the new MMS (mongodb?) version, constant Server selection timed out and locally many test fail too.
also in a debug run in CI it seems like the server somehow has a lot of failed hearbeats, and i dont understand why

even when testing locally against a mongodb docker container (via MONGOOSE_TEST_URI), those same tests fail and the final error logged also is Server selection timed out after 30000 ms

@hasezoey
Copy link
Collaborator Author

i still dont know what the problem is, but my theories are:

  • mongodb somehow does not refresh the connection in deno
  • mongodb does somehow not clear the selection timeout in deno
  • there is a connection that is not closed?
  • something takes longer than all the timeouts in deno, and so cannot complete it in time?
  • those errors exist in nodejs, but deno exits at the first one (and node just ignores it?)

@hasezoey
Copy link
Collaborator Author

if all tests run, it is mostly consistent which tests fail, the problem is that if they (the test file) is run individually, all tests of that run suite pass, but there will still be a server selection timed out message at the very end (even if all tests passed and all hooks passed too)

another finding:
it seems like it really does not like using populate, as soon as more than 1 path is populated, the test times out (even if timeout is set to 1 minute), example test:

const query = B.findById(b1).
populate('fans _creator embed.other embed.array embed.nested.subdoc').
populate({ path: 'adhoc.subdoc', model: 'User' }).
populate({ path: 'adhoc.subarray.things', model: 'User' });
const doc = await query.exec();

this test fails once it its the .exec and only if all the populates are removed it goes beyond that

it also "clogs up" any operation that comes after it (likely because it is not cleared by mongodb?)

also (which is likely related to the previous issue), deno seemingly messes up some kind of stream, error:

MongoParseError: Invalid message size: 1701593138, max allowed: 67108864

@hasezoey
Copy link
Collaborator Author

hasezoey commented Sep 1, 2023

i just found denoland/deno#19831, which is the exact issue this PR is having, greatly it is already fixed (by denoland/deno#20314), downside is that no deno version has been released yet and only git:master works with this code

PR Blocked until deno releases a version with this fix

@hasezoey
Copy link
Collaborator Author

deno 1.37.0 released, which included the fix, have pushed the update and the tests seem to be passing now again without problems, will clean-up and update PR tomorrow (or something)

but somehow there are no tests on this PR itself, only tests on the commit (on the fork)

@hasezoey
Copy link
Collaborator Author

updated PR to be based on latest master and cleaned it up

*also accidentally rebased once on 8.0 instead of master

this PR now also includes a fix for test file schema.select.test.js because somehow in deno some other test leaks a _id: immutable index and so errors the test, but i dont know which other test does that or how to properly fix this, so there is only a band-aid fix

@hasezoey
Copy link
Collaborator Author

hasezoey commented Oct 6, 2023

be41538 is the final test before releasing mongodb-memory-server 9, to ensure it also works outside of its own test-suite in its current state

will clean this PR up once it is released

@hasezoey hasezoey marked this pull request as ready for review October 6, 2023 18:43
@hasezoey hasezoey added the dependencies Pull requests that update a dependency file label Oct 6, 2023
@hasezoey
Copy link
Collaborator Author

hasezoey commented Oct 6, 2023

before being merged i would like to know if d8e1f4f is the best way or if someone maybe knows where the problem there is

@hasezoey
Copy link
Collaborator Author

hasezoey commented Oct 9, 2023

just for reference because it does not show up here, @vkarpov15 wrote on d8e1f4f:

Likely better to just drop the 'tests' collection. clearTestData() assumes that it runs after a test is done and clears all data related to models that test created.

@hasezoey
Copy link
Collaborator Author

hasezoey commented Oct 9, 2023

Updated to use db.dropDatabase directly, to my understanding clearTestData currently is just a more robust way of calling dropDatabase (retrying if failed), though i can understand if it has a different meaning and so could change in the future.

just as a note, i could not just do the db.model first and then call Test.collection.drop() because somehow that resulted in a error MongoServerError: ns not found.

@hasezoey hasezoey changed the base branch from master to 7.x October 25, 2023 08:12
@hasezoey
Copy link
Collaborator Author

changed base branch to 7.x (because of 8.0 merge), if this branch should not get merged over to 8.x, i will make another PR for that.

@hasezoey
Copy link
Collaborator Author

@vkarpov15 just as a reminder, this PR is ready to be merged.

it is targeting 7.x as the PR had been opened before 8.0 was even merged, and changed to 7.x (instead of staying 8.0) because this means mongodb 7.0 can be tested in mongoose 7.x and because the dependencies (nodejs and mongodb and others) align. if wanted i could also re-target to only have this updated in 8.x..

also if wanted i could rebase this PR to remove the merge commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants