Skip to content

Conversation

dariakp
Copy link
Contributor

@dariakp dariakp commented Dec 1, 2021

Description

NODE-3717 - part 1

What is changing?

Moving gridfs and csfle tests from functional to integration. Broke out gridfs spec tests into their own file. Made sure to update the csfle script to reference the new location and confirmed that the csfle test count in evergreen matches their count in main. Made sure the gridfs tests all still run.

Created temporary copies of the legacy spec runner in tools and the shared.js file in integration. Didn't want to touch every other file that's still in /functional by actually moving them. The duplicates that are in functional can be deleted once the entire suite is moved over. I'd love to get rid of the shared.js file altogether, but that would require more invasive refactoring of the individual tests than we presently have time for.

Is there new documentation needed for these changes?

Nothing beyond the update included in this PR to the test readme which explains the integration/functional test split as well as the spec/prose test file naming conventions.

What is the motivation for this change?

Sane organization of our test suite.

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 Team Review Needs review from team label Dec 1, 2021
@dariakp dariakp changed the title refactor(NODE-3713): move gridfs and csfle tests to the integration directory refactor(NODE-3717): move gridfs and csfle tests to the integration directory Dec 1, 2021
ljhaywar
ljhaywar previously approved these changes Dec 2, 2021
Copy link
Contributor

@ljhaywar ljhaywar left a comment

Choose a reason for hiding this comment

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

Looks good! The test readme changes made sense to me. Since you said this was moving existing tests, I didn't look through the specifics of each test.

One question: the deadlock_tests.js file references https://github.com/mongodb/specifications/blob/b3beada72ae1c992294ae6a8eea572003a274c35/source/client-side-encryption/tests/README.rst#deadlock-tests. Should that file name have .prose in it?

| TypeScript Definition | `/test/types` | The TypeScript definition tests verify the type definitions are correct. | `npm run check:tsd` |
| Type of Test | Test Location | About the Tests | How to Run Tests |
| ----------------------- | ------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| Unit | `/test/unit` | The unit tests test individual pieces of code, typically functions. These tests do **not** interact with a real database, so mocks are used instead. <br><br>The unit test directory mirrors the `/src` directory structure with test file names matching the source file names of the code they test. | `npm run check:unit` |
Copy link
Contributor

Choose a reason for hiding this comment

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

Love this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! And yeah, good call out on the deadlock tests, I didn't look very closely but I think the deadlock tests can be combined into the prose tests (like Neal also pointed out on the thread). The specific file is just the input to the tests, so I think I can rename it to better reflect that fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed the update

baileympearson
baileympearson previously approved these changes Dec 2, 2021
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

I didn't evaluate the tests for accuracy but overall the changes look good and the tests are passing. LGTM

Co-authored-by: Lauren Schaefer <lauren.schaefer@mongodb.com>
@dariakp dariakp dismissed stale reviews from baileympearson and ljhaywar via e62f990 December 2, 2021 15:42
@dariakp dariakp merged commit 04db406 into main Dec 2, 2021
@dariakp dariakp deleted the NODE-3713/reorganize-tests-pt1-gridfs-csfle branch December 2, 2021 17:23
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

Development

Successfully merging this pull request may close these issues.

4 participants