----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44947/#review126334 -----------------------------------------------------------
src/tests/cluster.cpp (lines 434 - 436) <https://reviews.apache.org/r/44947/#comment189583> This can be tackled in another review because a) the tests here don't use cluster.cpp and b) I see other issuse here as well (see below). src/tests/cluster.cpp (line 455) <https://reviews.apache.org/r/44947/#comment189584> Here AWAIT could result in a failure that terminates the lambda early and leave behind orphan containers. src/tests/containerizer/xfs_quota_tests.cpp (line 71) <https://reviews.apache.org/r/44947/#comment189492> We have `makeQuotaInfo` vs. `mkfile` & `mkloop`. Can we make the use of the word `make` or `mk` consistent? Generally we avoid abbreviations so using `make` (and camelCasing) is preferred. src/tests/containerizer/xfs_quota_tests.cpp (line 75) <https://reviews.apache.org/r/44947/#comment189302> No space: `{limit, used};` src/tests/containerizer/xfs_quota_tests.cpp (line 215) <https://reviews.apache.org/r/44947/#comment189483> This test can be extended to include files: ``` directory | |- file ``` You can call `set|clearProjectId()` on the directory and verify `getProjectId()` on the file inside (and the directory). src/tests/containerizer/xfs_quota_tests.cpp (line 217) <https://reviews.apache.org/r/44947/#comment189491> `s/p/prid/` or `s/p/project/` here and elsewhere. `project` is probably better because you use `projectA` and `projectB` below in another test. When there are more variables in scope, single letter variables can be confusing: e.g., `p` vs. `path` which also starts with `p`. src/tests/containerizer/xfs_quota_tests.cpp (line 218) <https://reviews.apache.org/r/44947/#comment189482> In your `setUp()` you have `chdir`d so it should save us from needing to spell out the full path here. Fix other occurrences as well. src/tests/containerizer/xfs_quota_tests.cpp (lines 222 - 223) <https://reviews.apache.org/r/44947/#comment189489> This can be done in one line: ``` EXPECT_NONE(getProjectId(path)); ``` Please fix other occurences at well. src/tests/containerizer/xfs_quota_tests.cpp (line 239) <https://reviews.apache.org/r/44947/#comment189484> It's now hard to do this test with the public `setProjectId()` no longer applicable to individual files but I think it's OK because you can construct the file hiearachies like above to achieve all achievable cases we'll run into. In principle testing against public APIs is sufficient and if we have enumerated all possiblilties of public API usage and still cannot cover all 100% paths in the private methods, then these paths are probably invalid and we should actually remove them. :) src/tests/containerizer/xfs_quota_tests.cpp (lines 263 - 264) <https://reviews.apache.org/r/44947/#comment189486> I guess this is saying that when O_NOFOLLOW is used along, `openPath()` would fail; when O_NOFOLLOW + O_PATH is used, `openPath()` would succeed but the resulting fd is very limited. I think these comments are better suitable to be put above the definition of `openPath()` than here because the test here doesn't have access to the `open` flags and the reader of the test aren't necessarily aware of the internal details. src/tests/containerizer/xfs_quota_tests.cpp (line 265) <https://reviews.apache.org/r/44947/#comment189485> s/used/use/ src/tests/containerizer/xfs_quota_tests.cpp (lines 273 - 280) <https://reviews.apache.org/r/44947/#comment189488> So for this we can also create ``` directory | |- link ``` Then: ``` getProjectId(link); setProjectId(directory); getProjectId(link); clearProjectId(directory); getProjectId(link); ``` Even if the reason for `EXPECT_ERROR(getProjectId(path));` to pass is due to FTS_PHYSICAL (so we didn't attempt to set it on the link instead of we cannot set it on the link), that's the situation we are ever going to run into in real scnearios anyways so it's OK. src/tests/containerizer/xfs_quota_tests.cpp (line 284) <https://reviews.apache.org/r/44947/#comment189508> This is the most comprehensive test in this file and it has covered all methods and not just for `AssignProjectId`: call it `DirectoryTree`? src/tests/containerizer/xfs_quota_tests.cpp (lines 292 - 294) <https://reviews.apache.org/r/44947/#comment189429> Instead of doing cleanups here, we can register the projectIds with a member variable: ``` vector<prid_t> projectIds; ``` This way in the `tearDown()` method we can go through all the them and call `clearProjectQuota()` on each. src/tests/containerizer/xfs_quota_tests.cpp (line 321) <https://reviews.apache.org/r/44947/#comment189493> ``` EXPECT_SOME_EQ( makeQuotaInfo(limit, Megabytes(2)), getProjectQuota(rootA, projectA)); ``` Here and below. src/tests/containerizer/xfs_quota_tests.cpp (lines 348 - 350) <https://reviews.apache.org/r/44947/#comment189497> This should be taken care of by TearDown(). src/tests/containerizer/xfs_quota_tests.cpp (lines 359 - 363) <https://reviews.apache.org/r/44947/#comment189498> ``` EXPECT_SOME_EQ( makeQuotaInfo(limit, Bytes(0)), getProjectQuota(path, projectId)); ``` As you did above. src/tests/containerizer/xfs_quota_tests.cpp (line 371) <https://reviews.apache.org/r/44947/#comment189502> s/projectId/process/ for consistency. src/tests/containerizer/xfs_quota_tests.cpp (line 386) <https://reviews.apache.org/r/44947/#comment189504> Nit: s/in/to/? src/tests/containerizer/xfs_quota_tests.cpp (line 387) <https://reviews.apache.org/r/44947/#comment189501> Just ``` EXPECT_SOME(makeFile("file", used)); ``` would be sufficient because you only have one file. src/tests/containerizer/xfs_quota_tests.cpp (lines 390 - 391) <https://reviews.apache.org/r/44947/#comment189499> ``` EXPECT_SOME_EQ( makeQuotaInfo(limit, used), getProjectQuota(path, projectId)); ``` The style you used is in the style guide but shouldn't be used if it results in too much "jaggedness" (as per the guide). src/tests/containerizer/xfs_quota_tests.cpp (line 396) <https://reviews.apache.org/r/44947/#comment189500> Kill line. - Jiang Yan Xu On March 31, 2016, 5 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44947/ > ----------------------------------------------------------- > > (Updated March 31, 2016, 5 p.m.) > > > Review request for mesos, Jie Yu and Jiang Yan Xu. > > > Bugs: MESOS-4828 > https://issues.apache.org/jira/browse/MESOS-4828 > > > Repository: mesos > > > Description > ------- > > Add tests for XFS project quota utilities. > > > Diffs > ----- > > src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 > src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 > src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION > src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 > > Diff: https://reviews.apache.org/r/44947/diff/ > > > Testing > ------- > > Make check. Manual testing. These tests. > > > Thanks, > > James Peach > >
