mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 35728: Fix failing test: SlaveTest.ROOT_RunTaskWithCommandInfoWithUser.
Date Fri, 26 Jun 2015 02:30:46 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35728/#review89458
-----------------------------------------------------------

Ship it!


Nice work! So, running the command as root first will guarantee that lt-mesos-executor exists
before trying to run the task as the test-user `nobody`? We might be able to fix this in a
cleaner way, but this looks good enough to me. Just fix the few minor points I raised and
update the comments, and then I'll commit it.

Please fill out the "Testing Done" section to explain how you verified that this test failed
before your change and now passes.


src/tests/slave_tests.cpp (line 671)
<https://reviews.apache.org/r/35728/#comment142097>

    Not yours, but please fix: s/precense/presense/



src/tests/slave_tests.cpp (line 708)
<https://reviews.apache.org/r/35728/#comment142088>

    You shouldn't start the driver until after you've set up your expectation for receiving
offers, otherwise the scheduler might receive offers before your expectation is in place.



src/tests/slave_tests.cpp (line 715)
<https://reviews.apache.org/r/35728/#comment142087>

    You should be able to use the same `offers` variable here as down below.



src/tests/slave_tests.cpp (lines 723 - 727)
<https://reviews.apache.org/r/35728/#comment142105>

    Please clarify this comment. You're running `active-user-test-helper` first as root (current
os::user, since this is a `ROOT_` test), so that root can create `lt-mesos-executor` before
we try to run `active-user-test-helper` as the test_user (`nobody`), correct?



src/tests/slave_tests.cpp (lines 731 - 732)
<https://reviews.apache.org/r/35728/#comment142090>

    s/MergeFrom/CopyFrom/ (wherever possible, since there's actually nothing to merge it with
here)



src/tests/slave_tests.cpp (lines 735 - 737)
<https://reviews.apache.org/r/35728/#comment142106>

    Also, `EXPECT_EQ("root", user)`



src/tests/slave_tests.cpp (line 747)
<https://reviews.apache.org/r/35728/#comment142093>

    s/MergeFrom/CopyFrom/


- Adam B


On June 22, 2015, 4:34 a.m., haosdent huang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35728/
> -----------------------------------------------------------
> 
> (Updated June 22, 2015, 4:34 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-2199
>     https://issues.apache.org/jira/browse/MESOS-2199
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fix failing test: SlaveTest.ROOT_RunTaskWithCommandInfoWithUser.
> 
> 
> Diffs
> -----
> 
>   src/tests/slave_tests.cpp 50301983c674ef50a64294816db9587bf065aa9e 
> 
> Diff: https://reviews.apache.org/r/35728/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> haosdent huang
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message