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
chore: refactor config tests #3373
base: main
Are you sure you want to change the base?
Conversation
test/config/config.test.js
Outdated
|
||
process.env[option.envVar] = existingEnvValue | ||
agent.destroy() | ||
t.end() |
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.
nit: My preference would be that utility test functions do not call the t.end()
themselves. While I know this means that the tests below need to write t.end()
more times, it does mean that it is a bit easier to read a given test block:
test(..., t => {
...
})
and see if it explicitly calls t.end()
.
test/config/config.test.js
Outdated
return def.types && def.types.indexOf(type) !== -1 | ||
} | ||
|
||
function testEnvVar (t, option, value1, value2) { |
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.
nit: Perhaps calling this function assertEnvVar
or similar, which is more of a current naming pattern:
% rg 'function test' test | wc -l
5
% rg 'function assert' test | wc -l
50
const BOOL_OPTS_WITH_ENV_EXCLUDED = [ | ||
// This ones depend on other options so we should have specific tests | ||
'captureSpanStackTraces', | ||
'asyncHooks', | ||
// this one seems to have a bug | ||
'breakdownMetrics' | ||
] |
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.
It would be nice if this was data-driven eventually. I.e. if the set of config vars tested here was from an export of lib/config/schema.js
. Is that the eventual intention?
I see that is done for NUM
tests below.
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.
Really nice indeed. For now this 1st attempt is quite simple since the test only checks one option in isolation and both captureStackTraces
and asyncHooks
have dependencies. We could add a deps
property on the schema entries, this way the filter function is enough
options definition would become
{
name: ' nameOfTheConfig',
defaultValue: 'value', // defined if has a default
envVar: 'ENV_VAR_NAME', // defined if option can be set via ENV
deps: ['nameOfAnotherConfig'], // defined if there are dependencies on other configs
types: [ 'boolean' ], // defined if there is a normalizer method
centralConfig: ' central_config_name', // defined if it has a central config
}
and the filter
const BOOL_OPTS_WITH_ENV_AND_NO_DEPS = CONFIG_DEFINITIONS.filter(function (def) {
return ('envVar' in def) && !def.deps && hasType(def, 'boolean')
})
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 think it might be even better for discussions and improvements to have CONFIG_SCHEMA
written on a direct export instead of having in a procedural way. I'll keep the generator code just in case we need to generate it again
} = require('../../../lib/config/schema') | ||
|
||
// Create schema for tests | ||
const CONFIG_SCHEMA = Object.keys(DEFAULTS).reduce((acc, key) => { |
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.
Do you intend to define the CONFIG_SCHEMA
in "./lib/config/..." files eventually? I'm a little confused why we would have an intermediate schema object just defined in the tests.
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.
Indeed that's the intent although it may have some slight changes at the end. The place for CONFIG_SCHEMA
shall be ./lib/config/schema.js
I think this tests will give us more confidence to move to the new schema before doing the refactor (in which we will remove this file and use the actual schema)
Co-authored-by: Trent Mick <trent.mick@elastic.co>
Description
Move config tests to a dedicated folder and also tried to simplify them. The tests rely on an array of config definitions with the following shape:
Checklist