Skip to content

Conversation

mdb-ad
Copy link
Contributor

@mdb-ad mdb-ad commented Apr 8, 2025

Note: some legacy tests were skipped by the C driver. The unified version is also untested.

  • badQueries (skipped in C driver due to due to test containing $type and C driver failing to parse: CDRIVER-3387).
  • unsupportedCommand (skipped in C driver due to not having mapReduce helper)

C driver Evergreen

New tests depend on unified test runner changes from DRIVERS-3106.

@kevinAlbs kevinAlbs self-requested a review April 10, 2025 18:05
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Exciting. Initial comments.

@kevinAlbs kevinAlbs requested review from rozza and removed request for katcharov and jyemin June 10, 2025 19:29
@mdb-ad mdb-ad requested a review from kevinAlbs June 11, 2025 01:25
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM. @ShaneHarvey and @rozza may I request trying these tests in Python and/or Java? That might help gain more confidence in the migrated tests work in case there are quirks in the C driver test runner. Unified test format changes to add support for autoEncryptOpts are in DRIVERS-3106.

@kevinAlbs kevinAlbs removed the request for review from ShaneHarvey June 24, 2025 12:20
Copy link
Member

@rozza rozza 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 one request:

Could timeoutMS.yml tests increase the timeoutMS and blockTimeMS by a factor of 10 as these timeout on evergreen.

It appears the java legacy versions were skipped due to this.

Other than that - LGTM

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.

This is a little bit last minute but #1823 (comment) is considering implementing a minLibmongocryptVersion runOnRequirement for FLE tests. If we do accept that change to the unified test format, could we also add min libmongocrypt version requirements to the new tests in this PR?

@mdb-ad mdb-ad requested a review from rozza August 14, 2025 05:33
@mdb-ad mdb-ad requested a review from baileympearson August 21, 2025 05:51
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 reviewed the libmongocrypt version requirement specifically. All old FLE continue to pass on older versions of our bindings and the new tests are skipped, meaning these won't break Node's CI if we test an old bindings version against the latest driver.

Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

LGTM!

@mdb-ad mdb-ad merged commit 49ade88 into mongodb:master Aug 28, 2025
6 checks passed
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.

6 participants