-
Notifications
You must be signed in to change notification settings - Fork 1.8k
test(NODE-3288): sync command-monitoring spec tests #2837
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-3288): sync command-monitoring spec tests #2837
Conversation
filterOutCommands, | ||
ignoreNsNotFound | ||
} = require('./shared'); | ||
const { loadSpecTests } = require('../spec'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we rename this to command_monitoring.test.js
to match the directory name change? Or would you rather do that as part of the greater test cleanup? I'm fine either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about doing it, but (1) I wasn't sure if we had in progress work touching this file and what potential merge conflict issues that might cause and (2) there are a lot of references to APM throughout and as you pointed out, the greater test refactor later is likely going to change a lot of things around, so not sure it's worth doing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think it's fine to wait. LGTM.
da577c3
to
cbe0f89
Compare
Description
NODE-3288
see also #2838