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

chore(NODE-3717): test reorg penultimate part #3103

Merged
merged 29 commits into from Jan 13, 2022
Merged

Conversation

dariakp
Copy link
Contributor

@dariakp dariakp commented Jan 12, 2022

Description

NODE-3717

What is changing?

Many things - basically, everything except functional/core: notably, the aws test has been moved, so we should sanity check to make sure the configuration still runs ok.

A number of tests have been consolidated with existing integration and unit test files. Things that aren't explicitly spec-related were moved more or less wholesale into the node-specific directory: this includes operation examples and the general examples folder. We can clean that up further later, right now though the goal is to make it easier to determine spec compliance, and for that purpose these files can be safely ignored.

Is there new documentation needed for these changes?

N/A

What is the motivation for this change?

Get tests that test the same things next to each other so that it's easier to tell what we have.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • [N/A] Changes are covered by tests
  • [N/A] New TODOs have a related JIRA ticket

@dariakp dariakp added the wip label Jan 12, 2022
@dariakp dariakp changed the title WIP(NODE-3717): test reorg part next chore(NODE-3717): test reorg penultimate part Jan 12, 2022
@dariakp dariakp marked this pull request as ready for review January 12, 2022 23:53
@dariakp dariakp removed the wip label Jan 12, 2022
it('Should open a new MongoClient connection', {
metadata: {
requires: {
topology: ['single']
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot of changes in this file but any reason we wouldn't want to remove the topology restriction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if you look at the commit history, I did try to remove the topology restrictions resulting in them all failing against unsupported topologies: the tests would have to updated to correctly initialize against different topologies, and, given that I'm not certain that these tests are even worth keeping in their current form, I didn't want to go through the effort of figuring out exactly which changes are needed to work with which topologies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, the changes in the file are primarily in copying existing tests from elsewhere over into the file and indentation, not changing the tests themselves

@dariakp dariakp added the Team Review Needs review from team label Jan 13, 2022
@nbbeeken nbbeeken merged commit b3d9fb8 into main Jan 13, 2022
@nbbeeken nbbeeken deleted the NODE-3717/test-reorg-d-m branch January 13, 2022 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
3 participants