----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51880/#review151611 -----------------------------------------------------------
Overall I think the tests here can be made simpler and more direct and focus on auto-detection. - Tests like `PersistentVolumeTest` use `getSlaveResources()` to `getDiskResource()` (way simpler than ours here) to construct the agent resources but it's as a way to configure the agent. Here we are testing whether resource parsing and detection work so we should just directly construct the input text (simple string or JSON string). - Given `ResourcesTest` already covers/should cover the various edge cases for resource parsing, in this tests we can just focus on detection (by constructing input strings that require auto-detection). The logic that glues parsed resources together are also exercised by it. src/Makefile.am (line 2263) <https://reviews.apache.org/r/51880/#comment230439> How about `constainerizer_tests.cpp` so it's easy to establish the mapping from <component> to <component>_tests.cpp. `common_containerizer_tests.cpp` reads like there's a containerizer called common containerizer. src/tests/containerizer/common_containerizer_tests.cpp (line 40) <https://reviews.apache.org/r/51880/#comment230441> One empty line. src/tests/containerizer/common_containerizer_tests.cpp (line 46) <https://reviews.apache.org/r/51880/#comment230442> Here we are really just testing `resources()`. How about naming it `ContainerizerResourcesTests`. In the future if more logic goes into the `Containerizer` code, we can add sepearate test fixtures. (Probably more likely for the resource detection code to move out first.) src/tests/containerizer/common_containerizer_tests.cpp (lines 64 - 89) <https://reviews.apache.org/r/51880/#comment230458> Why are they not SetUp and TearDown? These special methods are executed even when tests fail which is what we want. If the thinking is that these mounts don't need to be in tests that don't involve disks with `DiskInfo`, then they can be a separate test, e.g., `ConainerizerDiskResourcesTests`. src/tests/containerizer/common_containerizer_tests.cpp (lines 67 - 71) <https://reviews.apache.org/r/51880/#comment230468> Why are they parameterized? Can we just set up a MOUNT disk and a PATH disk in `SetUp()`? src/tests/containerizer/common_containerizer_tests.cpp (line 70) <https://reviews.apache.org/r/51880/#comment230477> If we know the size is 10M, in the test we can verify the detected size (inside a `#ifdef __linux__`) right? src/tests/containerizer/common_containerizer_tests.cpp (line 73) <https://reviews.apache.org/r/51880/#comment230469> Add a caveat: the detected values for these mocked mounts are basically bogus but we will just check whether *a* value is detected. src/tests/containerizer/common_containerizer_tests.cpp (lines 101 - 110) <https://reviews.apache.org/r/51880/#comment230455> Instead of having this many knobs, which is very hard to decipher, we could just specify the input string directly in the tests. src/tests/containerizer/common_containerizer_tests.cpp (line 289) <https://reviews.apache.org/r/51880/#comment230456> We can just directly construct these JSON strings. 1) Missing resource: trivial, an empty string misses all predefined resources. 2) Missing value: e.g., ``` string jsonString = R"~( [ { "name": "mem", "type": "SCALAR" } ])~"; ``` 3)-5) touches no JSON specific logic so if they are tested with simple strings we don't strictly need to test them here. 3) Resources with value zero are not auto-detected. 4) Custom resources are not auto-detected. 5) Resources specified with normal values are not changed. With these JSON string it's very obvious what the input looks like, as opposed to have a helper method with many knobs. If we construct JSON directly here, we can basically in one JSON input have all of the cases (except complex disks which are tested separately) right? e.g., missing cpu, mem with no value, disk with explicit zero, explicitly specified ports, etc. This way we don't have to repeate the same test lines over and over. src/tests/containerizer/common_containerizer_tests.cpp (lines 371 - 377) <https://reviews.apache.org/r/51880/#comment230476> I don't see the need to have this many combinations of disk types (this test and the ones below). If the test case sets up a mount disk and a path disk, can we just test this setup with missing values from the JSON input? Sample input: ``` strings::format(R"~( [ { "name": "disk", "type": "SCALAR", "disk": { "source": { "type": "MOUNT", "mount": { "root": "%s" } } } }, { "name": "disk", "type": "SCALAR", "disk": { "source": { "type": "PATH", "mount": { "root": "%s" } } } } ])~", mountRoot, pathRoot); ``` src/tests/resources_tests.cpp (lines 606 - 634) <https://reviews.apache.org/r/51880/#comment230440> Two space indentation. - Jiang Yan Xu On Dec. 9, 2016, 5:19 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51880/ > ----------------------------------------------------------- > > (Updated Dec. 9, 2016, 5:19 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-6062 > https://issues.apache.org/jira/browse/MESOS-6062 > > > Repository: mesos > > > Description > ------- > > Added unit tests to determine disk size for MOUNT or PATH disks. > > > Diffs > ----- > > src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47 > src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION > src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 > > Diff: https://reviews.apache.org/r/51880/diff/ > > > Testing > ------- > > All tests including the additional tests in this RR passed. > > > Thanks, > > Anindya Sinha > >
