Fix free memory calculation.

base/process/process_metrics_unittest.cc

Index: base/process/process_metrics_unittest.cc
diff --git a/base/process/process_metrics_unittest.cc b/base/process/process_metrics_unittest.cc
index 3e059b48a45f565f0bbe6f12060a19e8ada834f6..279f2ff84ac9bf3e2c99e0eb7368b0a2990e1cc9 100644
--- a/base/process/process_metrics_unittest.cc
+++ b/base/process/process_metrics_unittest.cc
@@ -106,6 +106,7 @@ TEST_F(SystemMetricsTest, ParseMeminfo) {
   std::string valid_input1 =
     "MemTotal:        3981504 kB\n"
     "MemFree:          140764 kB\n"
+    "MemAvailable:    2746720 kB\n"

[Primiano Tucci (use gerrit), 2017/02/15 11:18:33]

since this is not avialable on all kernels we support would be great that this
test did cover the case of MemAvailable being absent,  checking that the
fallback logic is sane.

[Michael K. (Yandex Team), 2017/02/17 13:38:48]

When MemAvailable is absent, meminfo.available is zero.
("Process metrics" are "as is", there are no magic formulas etc).
What do you want to check?
I agree it would be great if we can check this logic: base/sys_info_linux.cc
It should be done in base/sys_info_unittest.cc
But I'm not sure what is the best way to do it... You may suggest.

[Primiano Tucci (use gerrit), 2017/02/20 14:13:27]

I want to check that when we are in kernels that don't produce MemAvailable,
AmountOfAvailablePhysicalMemory still returns a valid number != 0, which is
calculated using your fallback logic..
You arelady have valid_input1 and valid_input2 where only one has MemAvailable
(good)
I would like to see a test that uses that as a mock input for
AmountOfAvailablePhysicalMemory() and checks that we get the right number.

[Michael K. (Yandex Team), 2017/02/20 14:33:13]

Acknowledged.

[Primiano Tucci (use gerrit), 2017/03/08 12:36:41]

I might be missing something but... where did this happen?

[Michael K. (Yandex Team), 2017/03/09 07:21:15]

See the new SysInfoTest.AmountOfAvailablePhysicalMemory test in
base/sys_info_unittest.cc.

I've put the test there because it tests the code from the corresponding
base/sys_info.cc.
And it seems logical:
* here we test that we can parse system/proc data into a SystemMemoryInfoKB
structure. And we check various cases including whether MemAvailable is present
or not;
* there (in sys_info_unittest.cc) we test that AmountOfAvailablePhysicalMemory
(which depends on SystemMemoryInfoKB structure) works correctly in both cases:
whether MemAvailable is present or not.

Don't you agree it's ok?

[Primiano Tucci (use gerrit), 2017/03/09 10:24:03]

The problem of that test you wrote is that what covers really depends on the
kernel it's being run onto. So if there is a bug in the logic, we might or might
not spot it, depending on the bots where it runs, which is not desirable. So, to
be clear, it's good to have that test that to check that the api works
end-to-end but you still lack coverage in this file for the case we discussed
above.
 
Sorry maybe I haven't been clear. Let me try to rephrase my statement above.
The coverage I would like to see in this test is:
1) Given a fixed /proc/meminfo input (like the ones here in these lines) that
contains MemAvailable, check that the number returned is sound and matches
MemAvailable (- the math on active file file).
2) Same thing, but with another fixed /proc/meminfo input which does NOT have
the MemAvailable line, to check that the fallback logic doesn't get accidentally
broken.

Now, I see here coverage for 1) below, which is good. I still don't see any
coverage for 2.

[Michael K. (Yandex Team), 2017/03/09 10:40:56]

Not exactly. See below.
There is the coverage for case #2 in base/sys_info_unittest.cc.
I've made a comment there. Plz pay attention.

But Ok, I'll also add expectations on AmountOfAvailablePhysicalMemory here. It
isn't hard and will prevent possible questions by other people in the future.

     "Buffers:          116480 kB\n"
     "Cached:           406160 kB\n"
     "SwapCached:        21304 kB\n"
@@ -171,6 +172,7 @@ TEST_F(SystemMetricsTest, ParseMeminfo) {
   EXPECT_TRUE(ParseProcMeminfo(valid_input1, &meminfo));
   EXPECT_EQ(meminfo.total, 3981504);
   EXPECT_EQ(meminfo.free, 140764);
+  EXPECT_EQ(meminfo.available, 2746720);
   EXPECT_EQ(meminfo.buffers, 116480);
   EXPECT_EQ(meminfo.cached, 406160);
   EXPECT_EQ(meminfo.active_anon, 2972352);
@@ -180,6 +182,7 @@ TEST_F(SystemMetricsTest, ParseMeminfo) {
   EXPECT_EQ(meminfo.swap_total, 5832280);
   EXPECT_EQ(meminfo.swap_free, 3672368);
   EXPECT_EQ(meminfo.dirty, 184);
+  EXPECT_EQ(meminfo.reclaimable, 30936);
 #if defined(OS_CHROMEOS)
   EXPECT_EQ(meminfo.shmem, 140204);
   EXPECT_EQ(meminfo.slab, 54212);
@@ -341,7 +344,7 @@ TEST_F(SystemMetricsTest, TestNoNegativeCpuUsage) {
 
 #endif  // defined(OS_LINUX) || defined(OS_CHROMEOS)
 
-#if defined(OS_WIN) || (defined(OS_MACOSX) && !defined(OS_IOS)) || \
+#if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_IOS) || \
     defined(OS_LINUX) || defined(OS_ANDROID)
 TEST(SystemMetrics2Test, GetSystemMemoryInfo) {
   SystemMemoryInfoKB info;
@@ -349,7 +352,12 @@ TEST(SystemMetrics2Test, GetSystemMemoryInfo) {
 
   // Ensure each field received a value.
   EXPECT_GT(info.total, 0);
+#if defined(OS_WIN)
+  EXPECT_GT(info.available, 0);
+  EXPECT_GT(info.free + info.zero, 0);
+#else
   EXPECT_GT(info.free, 0);
+#endif
 #if defined(OS_LINUX) || defined(OS_ANDROID)
   EXPECT_GT(info.buffers, 0);
   EXPECT_GT(info.cached, 0);
@@ -370,6 +378,10 @@ TEST(SystemMetrics2Test, GetSystemMemoryInfo) {
   EXPECT_LT(info.inactive_file, info.total);
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
 
+#if defined(OS_MACOSX) || defined(OS_IOS)
+  EXPECT_GT(info.file_backed, 0);
+#endif
+
 #if defined(OS_CHROMEOS)
   // Chrome OS exposes shmem.
   EXPECT_GT(info.shmem, 0);
@@ -378,7 +390,7 @@ TEST(SystemMetrics2Test, GetSystemMemoryInfo) {
   // and gem_size cannot be tested here.
 #endif
 }
-#endif  // defined(OS_WIN) || (defined(OS_MACOSX) && !defined(OS_IOS)) ||
+#endif  // defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_IOS) ||
         // defined(OS_LINUX) || defined(OS_ANDROID)
 
 #if defined(OS_LINUX) || defined(OS_ANDROID)