Skip to content
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.

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

test(NODE-3817): refactor RunOn filter in legacy spec runner to use mocha hooks #3119

Merged
merged 12 commits into from
Feb 2, 2022

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Jan 28, 2022

Description

What is changing?

  • Removes the logic that duplicates tests in the legacy spec runner based on runOn spec
    • Legacy spec runner will now reshape the legacy runOn requirement to match the unified format and use the same function from the unified runner to calculate test skip
    • topologySatisfies can be called from a test or beforeEach hook
  • Moves our start up hooks, to Mocha's require format (--file flag is slated for removal)
    • The configuration.js hook is our old tools/runner/index.js file, it will detect if the run is for unit tests
      • Is the unit test detection logic strong enough? it uses the process.argv[2] string
      • No unit test detection, we have a config file that is used for integration runs, and unit tests use the default .mocharc.json file
    • prints a clear specification of the current detected environment
  • Retryable Writes had custom runOn parsing logic, inherited similiar changes to the legacy spec runner
  • SDAM integration tests explcitly skip on LB topologies
  • ADL - patches in an empty runOn array, they're the only legacy specs that don't have them
  • Converted the TestConfiguration to TS, corrected small issue with auth option making it on to the searchParams
  • Unified runner attaches skipReason found in test files to our mocha skipReason now
  • Snappy the "unit" test was hitting the session leak checker for using an after hook instead of an afterEach

What is the motivation for this change?

This makes our test run more clear since there is not longer duplicates of a test. For example a countDocuments spec test is the same regardless of what topology it is running against, so we shouldn't duplicate the test name and function based on all possible topology we'd want to run that test against.

Migrating our hooks to the require format is to help us keep up to date on mocha's expectations. Writing them in this format is a precurser to potentially running tests in parallel.

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>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

test/tools/runner/config.ts Show resolved Hide resolved
test/tools/runner/config.ts Show resolved Hide resolved
const { inspect } = require('util');

const IS_UNIT = process.argv.length >= 3 && process.argv[2].includes('unit');
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could create an environment variable and feed it into the tests, like IS_UNIT=true, instead of relying on parsing the test directory structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something we'll want everyone's eyes on, I'm cool with that approach, maybe more clarity like NO_SERVER=true. But also folks shouldn't have to do this locally so maybe there's some heuristic we can figure out here.

  • In the case of no server running:
    • I want to be able to run mocha test/unit without issue so this before hook has to return early.
    • I expect mocha test/integration to fail server selection
    • I expect mocha test/unit test/integration to either fail server selection, or crash tests that access this.configuration.x, since it will be undefined
  • In the case of a server running:
    • I want to be able to run mocha test/unit test/integration without issue. <- currently this would fail, cus my logic makes the hook return early, leaving configuration undefined
    • I want to run mocha test/unit without issue, in this case it will still connect to the server and define this.configuration it's just that the unit tests don't use it, extra work but no biggie
    • ofc int tests work as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dariakp @durran, now that this is in team review could use a meeting of minds here for best approach.
I haven't added an env var like bailey asked yet, that could be done in CI for our lint task, but I wouldn't want to folks to have to do that locally necessarily to run unit tests without a server up, but then again it isn't a huge hurdle.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we instead set up a different mocharc file for the integration directory vs the unit directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with the bespoke config option for int tests.

test/tools/runner/hooks/configuration.js Outdated Show resolved Hide resolved
test/tools/spec-runner/context.js Outdated Show resolved Hide resolved
test/tools/spec-runner/index.js Outdated Show resolved Hide resolved
baileympearson
baileympearson previously approved these changes Feb 1, 2022
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

Capturing some feedback that I started in a separate tab

test/tools/unified-spec-runner/runner.ts Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.evergreen/run-serverless-tests.sh Show resolved Hide resolved
@nbbeeken nbbeeken requested a review from dariakp February 1, 2022 21:50
@nbbeeken nbbeeken marked this pull request as ready for review February 1, 2022 21:55
dariakp
dariakp previously approved these changes Feb 1, 2022
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

LGTM :)

@baileympearson baileympearson merged commit 323bb8d into main Feb 2, 2022
@baileympearson baileympearson deleted the NODE-3817/legacy-runOn branch February 2, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
3 participants