Skip to content

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Jan 8, 2021

Using the unified spec test schema this scaffolding
outlines the structure of a unified runner. Most tests
are skipped by the runOn requirements or not implemented
errors thrown by empty operation functions.

NODE-2287

Base automatically changed from chore/add-unified-spec-tests to master January 11, 2021 16:25
@nbbeeken nbbeeken force-pushed the NODE-2287/UnifiedTestFormat branch from 7ebfbe3 to c6c94be Compare January 11, 2021 17:18
@nbbeeken nbbeeken requested review from durran and emadum January 11, 2021 17:21
@nbbeeken nbbeeken marked this pull request as ready for review January 11, 2021 17:21
return m;
}

getClient(key: string): UnifiedMongoClient {
Copy link
Member

Choose a reason for hiding this comment

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

I'm noticing a common pattern in the getters (getClient, getDatabase, getCollection, getSession). It seems there could be an opportunity here to refactor the common behaviour and reduce the code here... What do you think about creating a generic getEntity method that takes the key and type and then checks the value is of the provided type and throws the error? Then each of the other methods would become one-liners?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, each of these methods also means we'd have a different stack trace for each one even though it is the same error, I've consolidated the helpers into getEntity and mapOf and I think it's friendly enough we don't need the bespoke methods any more, getEntity with a type param works nicely!

return e;
}

databases(): Map<string, Db> {
Copy link
Member

Choose a reason for hiding this comment

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

I'm also seeing the same pattern for getting maps of certain types - I think this could also be refactored to turn databases, collections, sessions, buckets, etc into one-liners as well.

@nbbeeken nbbeeken requested a review from durran January 13, 2021 21:33
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

Suggested a few typo fixes in comments but LGTM 👍

context(String(unifiedSuite.description), function runUnifiedTest() {
for (const test of unifiedSuite.tests) {
it(String(test.description), async function runOneUnifiedTest() {
// Function call for indentation sake...
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment is a bit confusing and could be removed or reworded

ctx.defer(async () => await entities.cleanup());

// Workaround for SERVER-39704:
// a test runners MUST execute a non-transactional distinct command on
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// a test runners MUST execute a non-transactional distinct command on
// test runners MUST execute a non-transactional distinct command on

specialCheck(result, operation.expectResult);
} else {
for (const [resultKey, resultValue] of Object.entries(operation.expectResult)) {
// each key/value expectation can be is a special op
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// each key/value expectation can be is a special op
// each key/value expectation can be a special op

Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

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

Nice refactor! LGTM

Using the unfied spec test schema this scaffolding
outlines the structure of a unified runner. Most tests
are skipped by the runOn requirements or not implemented
errors thrown by empty operation functions.

NODE-2287
@nbbeeken nbbeeken force-pushed the NODE-2287/UnifiedTestFormat branch from 9d2524f to bec50e6 Compare January 14, 2021 20:21
@nbbeeken nbbeeken merged commit 0b47a01 into master Jan 15, 2021
@nbbeeken nbbeeken deleted the NODE-2287/UnifiedTestFormat branch January 15, 2021 15:23
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.

3 participants