This is an automatically generated e-mail. To reply, visit:

src/tests/cluster.cpp (lines 434 - 436)

    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)

    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)

    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)

    No space: `{limit, used};`

src/tests/containerizer/xfs_quota_tests.cpp (line 215)

    This test can be extended to include files:
      |- 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)

    `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)

    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)

    This can be done in one line:
    Please fix other occurences at well.

src/tests/containerizer/xfs_quota_tests.cpp (line 239)

    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)

    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)


src/tests/containerizer/xfs_quota_tests.cpp (lines 273 - 280)

    So for this we can also create
     |- 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)

    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)

    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)

        makeQuotaInfo(limit, Megabytes(2)),
        getProjectQuota(rootA, projectA));
    Here and below.

src/tests/containerizer/xfs_quota_tests.cpp (lines 348 - 350)

    This should be taken care of by TearDown().

src/tests/containerizer/xfs_quota_tests.cpp (lines 359 - 363)

        makeQuotaInfo(limit, Bytes(0)),
        getProjectQuota(path, projectId));
    As you did above.

src/tests/containerizer/xfs_quota_tests.cpp (line 371)

    s/projectId/process/ for consistency.

src/tests/containerizer/xfs_quota_tests.cpp (line 386)

    Nit: s/in/to/?

src/tests/containerizer/xfs_quota_tests.cpp (line 387)

    EXPECT_SOME(makeFile("file", used));
    would be sufficient because you only have one file.

src/tests/containerizer/xfs_quota_tests.cpp (lines 390 - 391)

        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)

    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

Reply via email to