-
Notifications
You must be signed in to change notification settings - Fork 49
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
Sequencer Config Validator - broken_config and extra_config tests necessarily fail #385
Comments
I think this is also included in your issue #386 |
I've taken a look, and it does look like there's something wrong. In my testing, the config loglevel later gets set to 300, but then expects a log entry with a loglevel of 200. @grafnu I think the loglevel needs to be set to 100, and also Pubber checked as I can pass the test with pubber, so it looks like it's not respecting the min_loglevel?
|
I have no doubts that there's lots of things wrong with it. Just
because "it worked for me" (when I was coding it up) doesn't mean "it's
right" or works in the general sense :-).
The "log level needs to be set to 100" is a bit of a broad statement, so my
answer is "maybe." There's a default setting (used by all tests), and then
test-specific settings, etc... I would argue that the default
setting should likely be 200 or 300 since many debug-level messages will be
platform-specific, and so generally not useful for the sequence testing. In
specific cases (like with config) it might make sense to have it set to 100
for that one test b/c there is a specific stipulation on a debug message.
…On Tue, Jul 12, 2022 at 7:37 AM Noureddine ***@***.***> wrote:
I've taken a look, and it does look like there's something wrong. In my
testing, the config loglevel later gets set to 300, but then expects a log
entry with a loglevel of 200.
@grafnu <https://github.com/grafnu> I think the loglevel needs to be set
to 100, and also Pubber checked as I can pass the test with pubber, so it
looks like it's not respecting the min_loglevel?
2022-07-12T14:28:36Z DEBUG sequencer Updated config:
{
"timestamp" : "2022-07-12T14:28:26Z",
"version" : "1.3.13-90-g62cf3663-dirty",
"system" : {
"min_loglevel" : 300,
"metrics_rate_sec" : 600
}
}
2022-07-12T14:28:36Z TRACE sequencer received local_config_2022-07-12T14:28:36Z:
{
"timestamp" : "2022-07-12T14:28:26Z",
"version" : "1.3.13-90-g62cf3663-dirty",
"system" : {
"min_loglevel" : 300,
"metrics_rate_sec" : 600
}
}
2022-07-12T14:28:36Z INFO sequencer finished waiting for device config reset
2022-07-12T14:28:36Z DEBUG sequencer system config extra field null
2022-07-12T14:28:36Z TRACE sequencer received local_system_2022-07-12T14:28:36Z:
{
"min_loglevel" : 300,
"metrics_rate_sec" : 600,
"nonce" : 1657636116412
}
2022-07-12T14:28:36Z DEBUG sequencer update config_system
2022-07-12T14:28:38Z INFO sequencer start waiting for waiting for log message system.config.receive level INFO
—
Reply to this email directly, view it on GitHub
<#385 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD322BTHDYRD3SIPYL3VTV7JLANCNFSM525LVUUA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In the class
com.google.daq.mqtt.validator.validations.ConfigValidator
, the tests broken_config and extra_config necessarily fail :In the test setup (
com.google.daq.mqtt.validator.SequenceValidator#setUp
), the fieldmin_loglevel
of the device config is set to 400.According to the documentation, this means that the device will not report log messages with a level greater than 400 (
Level.WARNING
).The tests broken_config and extra_config necessarily fail, because they expect to receive log messages with a level of 200 (
Level.INFO
). This is less than 400, so the device will not send log messages with a level of 200 and the tests necessarily fail.The text was updated successfully, but these errors were encountered: