sequencer tests for sample rate #518
Conversation
bin/test_itemized
Outdated
| @@ -14,6 +14,11 @@ GOLDEN_FILE=etc/test_itemized.out | |||
| rm -f $RESULTS_OUT $SEQUENCER_OUT $PUBBER_OUT | |||
| touch $RESULTS_OUT $SEQUENCER_OUT $PUBBER_OUT | |||
|
|
|||
| #pubber/bin/build | |||
There was a problem hiding this comment.
Accidental, was meant to be uncommented, thanks
| updateDiscoveryConfig(config.discovery); | ||
| extractEndpointBlobConfig(); | ||
| } else { | ||
| info(getTimestamp() + " defaulting empty config"); | ||
| actualInterval = DEFAULT_REPORT_SEC * 1000; | ||
| } | ||
| if (configuration.options.fixedSampleRate != null) { | ||
| actualInterval = configuration.options.fixedSampleRate * 1000; |
There was a problem hiding this comment.
a better coding style is to avoid mutable variables, and instead intoruce a new variable with the new maning, eith.
useInterval = configuration.options.fixedSampleRate == null ? actualInterval : configuration.options.fixedSampleRate * 1000
In the big picture it makes the code more readable because you don't need to keep track of when a variable might change. Mutable variables are only really necessary in loops, etc... (and this is why the variable was initially indicated as final, which forces that it's written ones, and exactly once)
There was a problem hiding this comment.
Thanks for the feedback, changed as recommended
| @@ -82,7 +82,7 @@ public abstract class SequenceBase { | |||
| public static final String EVENT_PREFIX = "event_"; | |||
| public static final String SYSTEM_EVENT_MESSAGE_BASE = "event_system"; | |||
| public static final int CONFIG_UPDATE_DELAY_MS = 2000; | |||
| public static final int NORM_TIMEOUT_MS = 120 * 1000; | |||
| public static final int NORM_TIMEOUT_MS = 300 * 1000; | |||
There was a problem hiding this comment.
Is this change still necessary? I'm worried about this because it starts to drastically increase the overall time. Is this because the new tests that you added have longer times to them but there's no way to individually specify an increased timeout?
There was a problem hiding this comment.
This test needed a longer time to run, but the global timeout overrides the test level timeout. So I increased the overall time, then put a timeout on every test to keep the time low
There was a problem hiding this comment.
Well, I hate this logic but it's not your fault! It should be the other way around... default is shorter and then if you want longer it's a special case!
There was a problem hiding this comment.
(Nothing needs to change here, I'm just ranting)
| @@ -32,7 +40,7 @@ public void system_last_update() { | |||
| }); | |||
| } | |||
|
|
|||
| @Test | |||
| @Test(timeout = 120000) | |||
There was a problem hiding this comment.
why special-case the shorter time?
There was a problem hiding this comment.
As above, increased the global timeout so I can have a longer test, and then reinstated the shorter timeout on all the other tests
| * Tests both sample_rate_sec and sample_limit_sec by defining two non-intersecting narrow | ||
| * ranges of both parameters, and ensuring telemetry is within this range. | ||
| */ | ||
| @Test(timeout = 240000) |
There was a problem hiding this comment.
is it possible to make the 240000 a NAMED_CONSTANT_MS? I'm not sure syntactically if it's allowed, but would help readability overall!
There was a problem hiding this comment.
Yes it works, added a class of constants e.g. FOUR_MINUTES_MS. Let me know that needs changing
| @@ -859,19 +859,22 @@ private void configHandler(Config config) { | |||
|
|
|||
| private void processConfigUpdate(Config config) { | |||
| final int actualInterval; | |||
| final int useInterval; | |||
There was a problem hiding this comment.
can declare at point of use in this case since there is no branch involved.
| final int actualInterval; | ||
| boolean hasSampleRate = pointsetConfig != null && pointsetConfig.sample_rate_sec != null; | ||
| boolean hasSampleLimit = pointsetConfig != null && pointsetConfig.sample_limit_sec != null; | ||
| //if(hasSampleRate && hasSampleLimit && pointsetConfig.sample_rate_sec < ) |
There was a problem hiding this comment.
accidentally committed, removed
| @@ -82,7 +82,7 @@ public abstract class SequenceBase { | |||
| public static final String EVENT_PREFIX = "event_"; | |||
| public static final String SYSTEM_EVENT_MESSAGE_BASE = "event_system"; | |||
| public static final int CONFIG_UPDATE_DELAY_MS = 2000; | |||
| public static final int NORM_TIMEOUT_MS = 120 * 1000; | |||
| public static final int NORM_TIMEOUT_MS = 300 * 1000; | |||
There was a problem hiding this comment.
Well, I hate this logic but it's not your fault! It should be the other way around... default is shorter and then if you want longer it's a special case!
|
Closing for now since it seems stale. Please re-open when it's ready to address again. |
Sequence test for changable sample rate (sample_rate_sec) and changable publish interval (change both sample_rate and sample_limit)
Status for invalid sample rates will follow in a new PR