Repository navigation
Conversation
`MemoryManager::Result` records `total_allocated_bytes` and `net_heap_growth` as totals across `memory_iterations` iterations, while `JSONReporter` only emitted `allocs_per_iter` and those raw totals without `memory_iterations`, preventing consumers from computing per-iteration byte counts. Emit `memory_iterations` in `JSONReporter` whenever a memory run occurs, and update `memory_manager_test` and `memory_results_gtest`. AI-assisted: this change was written with the help of an AI coding tool.
LebedevRI
left a comment
LebedevRI
left a comment
There was a problem hiding this comment.
I hadn't realised we weren't doing this already.
| } | ||
| } | ||
|
|
||
| TEST_F(MemoryResultsTest, JSONReporterIncludesMemoryMetrics) { |
There was a problem hiding this comment.
We don't have much gtest's, i'm a bit unconvinced if this should be/needs a gtest.
@dmah42 ?
There was a problem hiding this comment.
up to you - happy to remove if you prefer. I would personally prefer to have some kind of test that will show up if this ever breaks as i'm about to start relying on it :-)
There was a problem hiding this comment.
We generally prefer end-to-end tests like the change in that other test file ^.
There was a problem hiding this comment.
Right - I saw the other file, and I thought, I'd like to know that integer is the right number (not just 'any integer')- felt a bit less than the ideal amount of assurance I'd be after to just assert that it's an int. Again, happy to revert this file if preferred, just explaining where i was coming from. Certainly I use tests a lot to gain assurance that my AI-driven code is actually doing the right thing - I probably add more tests than most people.
There was a problem hiding this comment.
i'm quite happy to have gtests for low level behaviour checks. the end to end tests are good for checking the actual output matches expectations, but gtests are quick and more direct. in general i wish we had more gtests and relied less on the actual output, but the output is ultimately what folks care about not regressing on.
Background: I have a tool consuming this, and I'd like to know how many iterations, so I can divide total bytes by iterations.
MemoryManager::Resultrecordstotal_allocated_bytesandnet_heap_growthas totals acrossmemory_iterationsiterations, whileJSONReporteronly emittedallocs_per_iterand those raw totals withoutmemory_iterations, preventing consumers from computing per-iteration byte counts.Emit
memory_iterationsinJSONReporterwhenever a memory run occurs, and updatememory_manager_testandmemory_results_gtest.AI-assisted: this change was written with the help of an AI coding tool.