Fix JSON logger reporting duplicate values despite configuration #28
Conversation
There was a problem hiding this comment.
I've been trying to merge this PR, but it breaks the tests.
On building the current GitHub origin/master branch at ebe7120, the program caPutJsonLogTest runs all its 499 tests successfully.
After hand-merging this PR's origin/cherry-pick branch locally into origin/master, caPutJsonLogTest hangs after printing 352 of the expected 499 results.
Checking out the tip of the cherry-pick branch at e183395 caPutJsonLogTest expects to run 375 tests, but it hangs after printing 331 results.
It seems that some of the test code is incompatible with duplicate values not being logged. I applied this change to the cherry-pick branch:
diff --git a/test/caPutJsonLogTest.cpp b/test/caPutJsonLogTest.cpp
index b6411d2..e135a7a 100644
--- a/test/caPutJsonLogTest.cpp
+++ b/test/caPutJsonLogTest.cpp
@@ -881,7 +881,7 @@ MAIN(caPutJsonLogTests)
startIoc();
CaPutJsonLogTask *logger = CaPutJsonLogTask::getInstance();
if (logger == NULL) testAbort("Failed to initialize logger.");
- logger->initialize(logServerAddress.c_str(), caPutJsonLogOnChange);
+ logger->initialize(logServerAddress.c_str(), caPutJsonLogAll);
testDiag("Test IOC ready");
testIocReady.trigger();
The result runs to completion with all 375 tests passing.
It might be a good idea to try applying the change from this branch to earlier commits to find out if the problem appears in Matic's original or was added more recently.
It would also be good to add tests for the different settings; it's possible to switch modes at runtime so we don't have to fix all the tests, just ensure the setting in use is appropriate for each set of tests.
|
@anjohnson - Good catch. There definitely seems to be an issue here in comparing arrays of strings; the comparison only compares the first entry in the string array; for the given test, since the first string is unchanged the "on change" check fails, even though the array has changed. As near as I can tell, despite adding the Anyhow, there are further issues in here that need to be looked at. I'm tempted to apply your change for the time being and then create an issue to fix array put comparisons; I don't have a good sense of how much of a rabbit hole that will be. I agree that adding tests for multiple modes is a good idea, but again, rabbit hole... |
This check would stop the logging, but a better solution would be for the reconfiguration to shut down and/or restart the thread as appropriate.
For the implementation of the JSON logger, the array comparison was left out. As such, when checking if the value had changed, we would only compare the first of the values from an array. Thus a change from ``` [1, 2, 3] -> [1, 3, 4, 5, 6, 7] ``` would be listed as not changing, which is not the correct behaviour.
|
@anjohnson - I have fixed the array comparison, which fixes the tests without the need for the change you suggested above. I think it is true that we should add more varied tests, but I think it might be better to create a followup issue for that. |
|
Yea, thanks! |
This is (roughly) a cherry-pick of ce7bd90.
Fixes #30