Skip to content

test(NODE-5929): convert txn legacy spec tests #3987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 28, 2024
Merged

test(NODE-5929): convert txn legacy spec tests #3987

merged 1 commit into from
Feb 28, 2024

Conversation

durran
Copy link
Member

@durran durran commented Feb 8, 2024 •
edited
Loading

Description

Node verification of mongodb/specifications#1502 and mongodb/specifications#1505

What is changing?

Syncs all the transaction unified tests, removes the legacy tests, fixes read preference issues and error message comparison issues in the unified runner.

Is there new documentation needed for these changes?

None

What is the motivation for this change?

NODE-3299/DRIVERS-1709

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

@durran durran force-pushed the NODE-3299 branch 3 times, most recently from dfeb836 to 126abd9 Compare February 12, 2024 10:32
durran
durran commented Feb 12, 2024
View reviewed changes
test/tools/unified-spec-runner/match.ts
@@ -735,7 +735,9 @@ export function expectErrorCheck(
}

if (expected.errorContains != null) {
expect(error.message, expectMessage).to.include(expected.errorContains);
expect(error.message.toLowerCase(), expectMessage.toLowerCase()).to.include(
expected.errorContains.toLowerCase()
Copy link
Member Author

Choose a reason for hiding this comment

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

Unified test runner spec states this check should be case insensitive, so I just fixed it here as a few tests were failing because of it.

durran
durran commented Feb 12, 2024
View reviewed changes
test/tools/unified-spec-runner/operations.ts
@@ -497,7 +497,7 @@ operations.set('replaceOne', async ({ entities, operation }) => {

operations.set('startTransaction', async ({ entities, operation }) => {
const session = entities.getEntity('session', operation.object);
session.startTransaction();
session.startTransaction(operation.arguments);
Copy link
Member Author

Choose a reason for hiding this comment

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

One issue this update exposed was the args weren't getting passed to 'startTransaction`. I've also updated the test changes in this PR to address my comments in mongodb/specifications#1502

durran
durran commented Feb 12, 2024
View reviewed changes
test/integration/transactions/transactions.spec.test.ts
@@ -0,0 +1,29 @@
import * as path from 'path';
Copy link
Member Author

Choose a reason for hiding this comment

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

I converted the tests to typescript. Unskipped everything and re-verified everything was was getting skipped and filed 2 new tickets (NODE-5925 and NODE-5924) for 2 new bugs these tests exposed in the driver.

@durran durran mentioned this pull request Feb 12, 2024
Merged
4 tasks
@durran durran changed the title test(NODE-3299): convert txn legacy spec tests test(NODE-5929): convert txn legacy spec tests Feb 13, 2024
durran
durran commented Feb 13, 2024
View reviewed changes
test/integration/transactions/transactions.spec.test.ts
'transaction options inherited from defaultTransactionOptions',
'transaction options inherited from client',
'causal consistency disabled'
// TODO(NODE-5855) - Gone away after NODE-5929
Copy link
Member Author

Choose a reason for hiding this comment

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

The unified tests that are related to NODE-5855 are automatically skipped by the unified tests due to the fact that targetedFailPoint will not work with multiple mongoses in load balancer mode. So those tests only run on sharded clusters.

@durran durran added the Blocked Blocked on other work label Feb 14, 2024
@durran durran force-pushed the NODE-3299 branch 3 times, most recently from fdf7d5b to 97d13df Compare February 14, 2024 08:21
@durran
Copy link
Member Author

durran commented Feb 14, 2024 •
edited
Loading

Remaining random failures related to https://jira.mongodb.org/browse/DRIVERS-2816. Underneath the hood the extra command started events are from the txn aborting due to a migration conflict and then retrying. Note that I did not skip these as the failure happens on random tests, and did not want to skip all of them to cover it, but can to keep everything green.

@durran durran force-pushed the NODE-3299 branch 2 times, most recently from e871882 to f8d463a Compare February 14, 2024 10:28
@durran durran removed the Blocked Blocked on other work label Feb 14, 2024
@durran durran mentioned this pull request Feb 14, 2024
Merged
4 tasks
@durran durran marked this pull request as ready for review February 14, 2024 11:19
@nbbeeken nbbeeken self-assigned this Feb 14, 2024
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Feb 14, 2024
@nbbeeken nbbeeken self-requested a review February 14, 2024 20:07
nbbeeken
nbbeeken suggested changes Feb 14, 2024
View reviewed changes
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

quick nit, and let's figure out what to do with the too often flaky tests 🤔

test/tools/unified-spec-runner/entities.ts Outdated
Comment on lines 546 to 547
// In case the endSession logic didn't unpin the server.
// session.unpin({ force: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we just delete this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

test/integration/transactions-convenient-api/transactions-convenient-api.spec.test.ts Outdated

runUnifiedSuite(loadSpecTests(path.join('transactions-convenient-api', 'unified')), test => {
return SKIPPED_TESTS.includes(test.description)
? 'TODO(NODE-): Skipping failing transaction tests'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for the tests failing related to https://jira.mongodb.org/browse/DRIVERS-2816? Maybe we can skip or retry based on the outcome of the test? I know neither is easy given the structure of the unified runner... The Mocha context is passed to the runner and would be the same context given to an afterEach hook, maybe there's a way to communicate "unfailing" the test that way?

Copy link
Member Author

Choose a reason for hiding this comment

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

I took the path of least resistance and just skipped individually for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nbbeeken Hold off for review again I still see I have a few more to skip individually.

@durran durran force-pushed the NODE-3299 branch 3 times, most recently from 30e0843 to 032c3fc Compare February 19, 2024 11:16
@durran durran requested a review from nbbeeken February 19, 2024 11:49
nbbeeken
nbbeeken suggested changes Feb 21, 2024
View reviewed changes
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

putting in req changes so you can send it back to me when ready

@durran durran requested a review from nbbeeken February 28, 2024 14:19
@nbbeeken nbbeeken merged commit 1ca6269 into main Feb 28, 2024
@nbbeeken nbbeeken deleted the NODE-3299 branch February 28, 2024 17:33
aditi-khare-mongoDB pushed a commit that referenced this pull request Mar 6, 2024
@durran @aditi-khare-mongoDB
test(NODE-5929): convert txn legacy spec tests (#3987)
52ef091
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbbeeken nbbeeken nbbeeken approved these changes

Assignees

@nbbeeken nbbeeken

Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@durran @nbbeeken