Skip to content

test(NODE-7561): Skip QE "prefixPreview" and "suffixPreview" tests on server 9.0.0+#4934

Open
PavelSafronov wants to merge 14 commits intomainfrom
node-7561-skip-tests
Open

test(NODE-7561): Skip QE "prefixPreview" and "suffixPreview" tests on server 9.0.0+#4934
PavelSafronov wants to merge 14 commits intomainfrom
node-7561-skip-tests

Conversation

@PavelSafronov
Copy link
Copy Markdown
Contributor

Description

Summary of Changes

Notes for Reviewers

What is the motivation for this change?

Release Highlight

Release notes highlight

Double check the following

  • Lint is passing (npm run check:lint)
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@PavelSafronov PavelSafronov marked this pull request as ready for review May 2, 2026 01:46
@PavelSafronov PavelSafronov requested a review from a team as a code owner May 2, 2026 01:46
Copilot AI review requested due to automatic review settings May 2, 2026 01:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates CSFLE Queryable Encryption (QE) test coverage to avoid running deprecated prefixPreview / suffixPreview scenarios against MongoDB server versions where they’re no longer supported (9.0.0+), keeping CI green while the server/spec behavior diverges.

Changes:

  • Add maxServerVersion: 8.99.99 run-on requirements to QE unified spec tests that rely on prefixPreview / suffixPreview.
  • Fix mislabeled substringPreview test description in unified spec files.
  • Gate/skip integration test logic and spec-suite execution paths that would fail on MongoDB 9.0.0+.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/spec/client-side-encryption/tests/unified/QE-Text-suffixPreview.yml Restricts suffixPreview unified test to MongoDB < 9.0.0
test/spec/client-side-encryption/tests/unified/QE-Text-suffixPreview.json Same restriction in JSON form
test/spec/client-side-encryption/tests/unified/QE-Text-prefixPreview.yml Restricts prefixPreview unified test to MongoDB < 9.0.0
test/spec/client-side-encryption/tests/unified/QE-Text-prefixPreview.json Same restriction in JSON form
test/spec/client-side-encryption/tests/unified/QE-Text-compactStructuredEncryptionData.yml Restricts structured encryption test dependent on removed preview query types
test/spec/client-side-encryption/tests/unified/QE-Text-compactStructuredEncryptionData.json Same restriction in JSON form
test/spec/client-side-encryption/tests/unified/QE-Text-cleanupStructuredEncryptionData.yml Restricts cleanup structured encryption test dependent on removed preview query types
test/spec/client-side-encryption/tests/unified/QE-Text-cleanupStructuredEncryptionData.json Same restriction in JSON form
test/spec/client-side-encryption/tests/unified/QE-Text-substringPreview.yml Corrects test description label to substringPreview
test/spec/client-side-encryption/tests/unified/QE-Text-substringPreview.json Same description correction in JSON form
test/integration/client-side-encryption/client_side_encryption.prose.27.text_queries.test.ts Skips prefix/suffix setup and cases on MongoDB 9.0.0+
test/integration/change-streams/change_streams.spec.test.ts Attempts to version-gate unified spec suite for MongoDB 9.0.0+

Comment thread test/integration/change-streams/change_streams.spec.test.ts Outdated
Comment on lines +45 to +46
const isServer9OrAbove = this.configuration.version >= '9.0.0';
const shouldRunPrefixSuffixTests = !isServer9OrAbove;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated, using semver.

@tadjik1 tadjik1 self-assigned this May 4, 2026
@tadjik1 tadjik1 added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label May 4, 2026
// TODO: NODE-7559 - remove the mongodb version requirement once the spec tests are updated to be compatible with MongoDB 9.0
describe('Change Streams Spec - Unified', function () {
runUnifiedSuite(loadSpecTests(path.join('change-streams', 'unified')));
it('should run, unless it should not', { requires: { mongodb: '<9.0' } }, function () {
Copy link
Copy Markdown
Member

@tadjik1 tadjik1 May 4, 2026

Choose a reason for hiding this comment

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

There is one fundamental problem with this approach, caused by 2 independent causes:
describe (which is created by runUnifiedSuite) is ignored when defined inside it (introduced by this change), so you can see on evergreen logs like this:

[2026/05/02 03:55:27.933]   Change Streams Spec - Unified
[2026/05/02 03:55:27.933]     ✔ should run, unless it should not
[2026/05/02 03:55:28.266]   Client Backpressure (Prose)
[2026/05/02 03:55:28.266]     ✔ Test 1: Operation Retry Uses Exponential Backoff (325ms)
[2026/05/02 03:55:28.536]     ✔ Test 3: Overload Errors are Retried a Maximum of MAX_RETRIES times (259ms)
[2026/05/02 03:55:28.584]     ✔ Test 4: Overload Errors are Retried a Maximum of maxAdaptiveRetries times when configured

Basically it means no tests are running. But then there a second problem I want to highlight: in our current configuration the matrix looks like:

const MONGODB_VERSIONS = ['latest', 'rapid', '8.0', '7.0', '6.0', '5.0', '4.4', '4.2'];

Which means (with the constraints down below mongodb: '>=8.2.0 <9.0.0')we don't run tests on 8.1, 8.2, etc. The only variant we catch - is rapid, just because it's 8.3 currently. In other words we do not run tests on latest stable release which is 8.2.7 at the moment.

Can you please take a look into this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! PR updated, here are the new results:

The explanation is actually more complicated than MONGODB_VERSIONS.
(MONGODB_VERSIONS is only used to build the jobs. rapid results in the latest 8 version, while latest pulls in 9.0.0-alpha..., I don't think it's related to this at all.)

The last version was:

it('should run, unless it should not', { requires: { mongodb: '<9.0' } }, function () {
  runUnifiedSuite(loadSpecTests(...));
});

When MDB was < 9.0: Mocha runs the it callback, runUnifiedSuite tries to register test blocks during test execution, which Mocha silently drops. The outer it passes with zero assertions, giving us a false green. The change stream spec tests never run.
When MDB was >= 9.0: Mocha's requires check filters out the outer it: same thing, tests don't run.

New code:

describe('Change Streams Spec - Unified', function () {
  runUnifiedSuite(loadSpecTests(...), (_test, ctx) =>
    gte(ctx.version, '9.0.0') ? 'TODO(NODE-7559): ...' : false
  );
});

When MDB < 9.0: runUnifiedSuite called synchronously inside describe, so all nested/new context/it blocks register correctly during suite construction. Mocha then executes them normally.
When MDB >= 9.0: same new blocks are registered, then as each it executes, the skipFilter returns a skip reason string, and Mocha marks that individual test as skipped due to NODE-7559.

Copy link
Copy Markdown
Member

@tadjik1 tadjik1 left a comment

Choose a reason for hiding this comment

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

@PavelSafronov let's discuss the testing approach here as none of the affected tests seem running currently on CI.

Comment on lines +7 to +13
// TODO: NODE-7559 - remove once spec tests are compatible with MongoDB 9.0
describe('Change Streams Spec - Unified', function () {
runUnifiedSuite(loadSpecTests(path.join('change-streams', 'unified')));
runUnifiedSuite(loadSpecTests(path.join('change-streams', 'unified')), (_test, ctx) =>
gte(ctx.version, '9.0.0')
? 'TODO(NODE-7559): change stream spec tests not yet compatible with MongoDB >= 9.0'
: false
);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// TODO: NODE-7559 - remove once spec tests are compatible with MongoDB 9.0
describe('Change Streams Spec - Unified', function () {
runUnifiedSuite(loadSpecTests(path.join('change-streams', 'unified')));
runUnifiedSuite(loadSpecTests(path.join('change-streams', 'unified')), (_test, ctx) =>
gte(ctx.version, '9.0.0')
? 'TODO(NODE-7559): change stream spec tests not yet compatible with MongoDB >= 9.0'
: false
);
describe('Change Streams Spec - Unified', function () {
runUnifiedSuite(loadSpecTests(path.join('change-streams', 'unified')));

Sorry for the confusion - we actually don't need this anymore as we are 2nd implementer and there is already PR into spec to remove those tests. You can drop it in favor of #4935

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removing.

Comment on lines +48 to +50
console.log(
`Running tests for MongoDB version ${this.configuration.version}. Prefix/suffix tests should ${shouldRunPrefixSuffixTests ? '' : 'not'} run.`
);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
console.log(
`Running tests for MongoDB version ${this.configuration.version}. Prefix/suffix tests should ${shouldRunPrefixSuffixTests ? '' : 'not'} run.`
);

I guess this is leftover and can be removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removing.

Comment on lines +37 to +38
const isVersionCompatible = semver.satisfies(this.version, test.metadata.requires.mongodb);
return isVersionCompatible;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the purpose of this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Testing. Removing.

Copy link
Copy Markdown
Member

@tadjik1 tadjik1 left a comment

Choose a reason for hiding this comment

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

Thanks @PavelSafronov!

I left suggestion to remove changes in change streams (we can either merge them in a separate or, if you prefer, as part of this PR but as updated spec tests, please see #4935 for more details).
Also couple of leftovers and clarifications are needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Primary Review In Review with primary reviewer, not yet ready for team's eyes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants