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

Change Tests to use MMS over manually downloading & installing #12262

Merged
merged 18 commits into from Oct 3, 2022

Conversation

hasezoey
Copy link
Collaborator

Summary

This PR changes the Tests to use MMS instead of requiring a active instance and "manually" downloading in CI tests

This PR also does:

  • reset color after "No shard Testing" message
  • change from "mocha.opts" file to a ".mocharc.yml"
  • change "common.js"(test utils) to use URL generation based on MONGOOSE_TEST_URI for both uri and uri2
  • change some tests from assert.ok(number === 0) to assert.equal(number, 0) to get better error messages (expire after seconds)
  • change tests to always use start.uri over alias and static coding
  • update used 5.0.x version used in test.yml from 5.0.2 to 5.0.8

This PR is a DRAFT because it is still WIP, it is WIP because:

  • old code is commented out instead of removed (like setting up the replica sets & "test.yml" "manual" downloading)
  • tests fail because some tests expect journaling, which is not available with ephermeralForTest, and wiredTiger yet to speed up tests (current count of failing tests is 2)

re #12257 (comment)
re #12052 (somewhat)

@hasezoey
Copy link
Collaborator Author

hasezoey commented Aug 13, 2022

merged latest master, updated the used mongodb version to their latest patch version (4.0.2 -> 4.0.28, 5.0.2 -> 5.0.8)

though i am still unsure what to do with the tests that require journaling

PS: the test now should also run properly in the CI

@hasezoey
Copy link
Collaborator Author

hasezoey commented Sep 1, 2022

Updated to latest master and applied changes for #12356

Issues still to resolved:

  • there are tests which check something with journaling, but in-memory engine is used, what should be done about those tests?

@hasezoey
Copy link
Collaborator Author

hasezoey commented Sep 1, 2022

Updated to fix CI issues

Issues still to resolved:

  • there are tests which check something with journaling, but in-memory engine is used, what should be done about those tests?

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Looking good so far, I like this idea 👍

replseturi = '';
}

process.env.MONGOOSE_TEST_URI = instanceuri;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we set these as global variables rather than overwriting process.env? I get why setting these variables is necessary, I just really don't like overwriting process.env.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i overwrite a process.env variable, because that is the exact variable that was already used in test/common.js, and so was the easiest way of implementing it (i dont know if global carries over, at least in jest it does not)

Copy link
Collaborator

Choose a reason for hiding this comment

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

global does carry over in Mocha. That is one of many reasons why we advise people against using Jest: https://mongoosejs.com/docs/jest.html#globalsetup-and-globalteardown . Overwriting env var is more of an antipattern than overwriting global IMO, although I think it would be better to just export these.

test/mocha-fixtures.js Outdated Show resolved Hide resolved
}

if (startMemoryReplset) {
mongorreplset = await mms.MongoMemoryReplSet.create({ replSet: { count: 3, args: ['--setParameter', 'ttlMonitorSleepSecs=1'] } }); // using 3 because even numbers can lead to vote problems
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do count: 1? count: 3 takes longer to start up and we don't do things that would require count: 3, like testing replica set failover.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i changed it to 3 because the previous value was 2, and mongodb recommends to not use EVEN numbers, so instead of going down to 1 i had gone up to 2, see

count: 2,
for previous code

@hasezoey
Copy link
Collaborator Author

Update:

  • merged latest master
  • updated all MMS configurations to use storageEngine: 'wiredTiger'

i changed the storage-engine to be wiredTiger because this is the only engine that has journaling which is needed for some tests and it seems like mongodb is removing (or possibly renaming?) ephermerForTest in 6.1, but does not have yet any changelog entry (and it is also not mentioned in the mongodb documentation anymore from what i can tell)
tests still pass at ~40-60 seconds (normal instance + replset count 3)

@hasezoey hasezoey marked this pull request as ready for review September 25, 2022 08:03
@hasezoey hasezoey marked this pull request as draft September 25, 2022 08:12
@hasezoey
Copy link
Collaborator Author

actually convert it back to draft, because i am seeing some MMS startup errors, will try to fix those before un-drafting again

@hasezoey
Copy link
Collaborator Author

Updated MMS to fix the problems i had found, i think the test now failing is not related with changing to MMS

also as for performance:
locally, with now storage-engine wiredTiger and the directories used on a tmpfs, the times range from 40s to 60s
in the github CI it seems like it is not having a tmpfs, so it is slower, but not by much and the time is between 60s and 120s (mocha does not give more accurate times it seems once hitting a minute)

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

I had some nits about overwriting environment variables, but I don't think it is worth blocking this PR any more. This PR is pretty great, I love reducing the amount of misc download scripts we have. Can patch that up later as time allows.

@vkarpov15 vkarpov15 added this to the 6.6.4 milestone Oct 3, 2022
@vkarpov15 vkarpov15 merged commit b8567c3 into Automattic:master Oct 3, 2022
@hasezoey hasezoey deleted the changeTestToMMS branch October 3, 2022 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants