chore: another batch of config tests being refactored #3762
Conversation
github-actions
bot
added
the
agent-nodejs
| @@ -34,7 +34,7 @@ function normalizeKeyValuePairs(opts, fields, defaults, logger) { | |||
| if (key in opts) { | |||
| if (typeof opts[key] === 'object' && !Array.isArray(opts[key])) { | |||
| opts[key] = Object.entries(opts[key]); | |||
| return; | |||
| continue; | |||
There was a problem hiding this comment.
Note for reviewer: Tests revealed this return statement was doing an early finish of the normalization process leaving other options without normalization
| 'Elastic APM agent disabled (`active` is false)', | ||
| 'got a debug log about agent disabled', | ||
| ); | ||
| // XXX: normalization turns this value to `undefined` because "bogus" |
There was a problem hiding this comment.
Note to reviewer: since undefined is falsy we may want to actually have false so we ensure we have the right type
There was a problem hiding this comment.
I think undefined should be treated the same as "not specified". What happens if the same config option is specified at multiple levels, e.g.: ELASTIC_APM_ACTIVE=nope and apm.start({ active: true }). Normally the envvar is a high priority, so it should win. But in this case, I think we'd expect to see:
- a warning that the
nopevalue is invalid, and hence ignored - active is
truebecause the value fromapm.start(...)was used.
I'm not sure if that answers your question. :)
There was a problem hiding this comment.
as discussed this should be changed to follow the spec (which you summarised here). Since it is a change of behaviour is out of the scope of this PR. I'll create an issue for this change
There was a problem hiding this comment.
nit: Can we avoid commiting "XXX", or at least for very long, please? :) That's a string I use heavily as a marker for "this code is for local debugging/dev notes that is incomplete and shouldn't go in the release".
There was a problem hiding this comment.
sorry I left this on the code :(
I'll merge this one and create a new PR today moving more tests and removing this comment.
test/config/config.test.js
Outdated
| @@ -43,6 +43,19 @@ function getUseAgentLogs(stdout) { | |||
| .map((l) => l.replace(prefix, '')); | |||
| } | |||
|
|
|||
| /** | |||
| * Returns the warnings emitted by the logger | |||
There was a problem hiding this comment.
nit: The description isn't quite accurate about it being specifically about "warn" level.
| 'Elastic APM agent disabled (`active` is false)', | ||
| 'got a debug log about agent disabled', | ||
| ); | ||
| // XXX: normalization turns this value to `undefined` because "bogus" |
There was a problem hiding this comment.
I think undefined should be treated the same as "not specified". What happens if the same config option is specified at multiple levels, e.g.: ELASTIC_APM_ACTIVE=nope and apm.start({ active: true }). Normally the envvar is a high priority, so it should win. But in this case, I think we'd expect to see:
- a warning that the
nopevalue is invalid, and hence ignored - active is
truebecause the value fromapm.start(...)was used.
I'm not sure if that answers your question. :)
| 'Elastic APM agent disabled (`active` is false)', | ||
| 'got a debug log about agent disabled', | ||
| ); | ||
| // XXX: normalization turns this value to `undefined` because "bogus" |
There was a problem hiding this comment.
nit: Can we avoid commiting "XXX", or at least for very long, please? :) That's a string I use heavily as a marker for "this code is for local debugging/dev notes that is incomplete and shouldn't go in the release".
move & refactor tests for: - byte and other formats and special values - kube env vars options - key/value pairs options
This PR refactors tests for:
Checklist