test(NODE-4663): add csfle prose test 20

[baileympearson]

Description

What is changing?

This PR adds CSFLE prose test 20.

Is there new documentation needed for these changes?

What is the motivation for this change?

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
  • Example: feat(NODE-1234)!: rewriting everything in coffeescript
• Changes are covered by tests
• New TODOs have a related JIRA ticket

[dariakp]

Just curious why we went with storing and accessing this info via static props instead of accessing it from the test suite context.

It looks like the ClientSideEncryptionFilter initialize method does add it to the context in addition to storing it on the class, so it wasn't immediately obvious to me why we need both.

    context.clientSideEncryption = {
      enabled: this.enabled,
      mongodbClientEncryption,
      version: ClientSideEncryptionFilter.version,
      libmongocrypt: ClientSideEncryptionFilter.libmongocrypt,
      cryptShared: ClientSideEncryptionFilter.cryptShared
    };

[baileympearson]

I didn't realize we also stored it on the test configuration. I can adjust the implementation, if you're prefer that approach.

[dariakp]

I just think it's best to aim for consistency, though perhaps it's out of scope for this PR. What I see is that the ClientSideEncryptionFilter is the only filter that uses static properties in addition to setting context (compare to MongoDBVersionFilter or MongoDBTopologyFilter), and the tests that rely on that filter seem to primarily use those static accessors, with the notable exception of unified-utils, which reads the enabled property from the context instead.

So, if we want to be generally consistent with the rest of the filters, we'd probably want to remove the static accessors and rely on the context. But if we want to be consistent within the use of just this filter, then we'd want to remove the context setting and just use the static props.

It's a hard call because while I think that reading this info from context is somewhat more intuitive than reading it from the filters (since the filters are just one of the mechanisms of extracting environment info, and the rest of the environment info gets pushed onto the context), reading the info explicitly from the individual filters makes it easier to track relevant usage in our test code.

What do you think?

[baileympearson]

Sure, I'm happy to make this change. I don't think it would be too much additional work.

Looking at the tests, I did find

node-mongodb-native/test/integration/node-specific/auto_encrypter.test.ts

Line 15 in 3faf0c9

export const cryptShared = (status: 'enabled' | 'disabled') => () => {

. This is used in tandem with the predicate filter to skip tests without requiring a beforeEach hook (which in turn requires a describe around it):

it(
    'should autoSpawn a mongocryptd on init by default',
    { requires: { clientSideEncryption: true, predicate: cryptShared('disabled') } },
    async function () {
     ...
    }
  );

this.configuration isn't available in the predicate, so we use the cryptShared static property instead.

Maybe it makes sense to

1. remove all static properties from the ClientEncryption filter for consistency with other filters
2. define a filter that filters for crypt shared loaded / not loaded (requires: { crypt_shared: 'loaded' })

Thoughts? I can make those changes if we want, I don't think it would take much effort or expand the scope of this work much.

[dariakp]

That works for me, I support quick wins for consistency :)