Fix parsing overflow issue (#24) #25
Conversation
Update system cpu measurement name to reflect units Signed-off-by: Devin Bonnie <dbbonnie@amazon.com>
| @@ -34,6 +34,8 @@ constexpr const char proc_sample_2[] = | |||
| constexpr const std::chrono::milliseconds TEST_PERIOD = | |||
| std::chrono::milliseconds(50); | |||
| constexpr const double CPU_ACTIVE_PERCENTAGE = 2.7239908106334099; | |||
| constexpr const char proc_sample_resolution_test[] = | |||
There was a problem hiding this comment.
Why not put this (and other proc_samples) into test_constants.hpp?
There was a problem hiding this comment.
Why not put this (and other
proc_samples) intotest_constants.hpp?
I created the test constants file for the shared constants between tests, but didn't have a strong opinion about the location of the constants in question. For consistency I think it makes sense to move them so will do so - thanks for the suggestion.
There was a problem hiding this comment.
I did suggest it for consistency.
However, if test_constants.hpp were a clean slate, my first thought would be to keep the memory strings in test_linux_memory_measurement.cpp and the CPU strings kept here in test_linux_cpu_measurement.cpp, but things like TEST_PERIOD and INVALID_PUBLISH_WINDOW be moved to test_constants.hpp.
With my upcoming unit tests, there will be more things to put into test_constants.hpp, so don't worry about it being under-utilized.
There was a problem hiding this comment.
seems to me like it'd make sense to just put this in the test itself, until it's actually being shared...?
There was a problem hiding this comment.
TEST_PERIOD and INVALID_PUBLISH_WINDOW are examples of what's already common between test_linux_memory_measurement.cpp and test_linux_cpu_measurement.cpp.
There was a problem hiding this comment.
FYI the constants defined in the test_constants.hpp file are shared. Since none of the CPU constants are currently shared I'll forgo moving them and merge this PR.
Signed-off-by: Devin Bonnie dbbonnie@amazon.com