> On Oct. 23, 2014, 10:48 p.m., Jie Yu wrote: > > src/tests/slave_tests.cpp, lines 431-436 > > <https://reviews.apache.org/r/26622/diff/2/?file=719278#file719278line431> > > > > It would be great if you can write an end-to-end test so that we can > > verify that the 'command' actually runs correctly with shell=false and has > > an argument list (refer to the following test). > > R.B. Boyer wrote: > I would love to try writing an e2e test to test this, but I spent several > hours on it before caving and just writing this GetExecutorInfo test. I have > about zero experience writing C++ code let alone C++ unit tests so I was > fighting with the project more than fighting with the bug. > > Timothy Chen wrote: > You can pretty much copy the MesosExecutorWithOverride test in > slave_tests.cpp, and just modify the CommandInfo accordingly. Let us know if > you still don't know what to do and spend too much time figuring out. > > R.B. Boyer wrote: > I think what I had trouble with there when last I was working on that was > actually asserting that mesos-executor died (exit != 0) when passed > double-dash arguments. I could get the test to pass, but it also passed > without the fix. > > R.B. Boyer wrote: > So this is the part of that unit test I have no idea how to adapt: > > // Expect the launch and just assume it was sucessful since we'll be > > // launching the executor ourselves manually below. > > Future<Nothing> launch; > EXPECT_CALL(containerizer, launch(_, _, _, _, _, _, _)) > .WillOnce(DoAll(FutureSatisfy(&launch), > Return(true))); > ...and more ... > > In my case I need to strongly assert that the command launched without > failing (which requires a mesos-executor binary to actually run) OR inject a > Capture of some sort here and trap the exec to assert it is sane without > finishing the e2e part of this.
Ah ok so you should look at this test instead (ROOT_RunTaskWithCommandInfoWithoutUser) which simply launches a real slave and run through the executor launch. - Timothy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26622/#review58149 ----------------------------------------------------------- On Oct. 18, 2014, 4:40 a.m., R.B. Boyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26622/ > ----------------------------------------------------------- > > (Updated Oct. 18, 2014, 4:40 a.m.) > > > Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen. > > > Bugs: MESOS-1873 > https://issues.apache.org/jira/browse/MESOS-1873 > > > Repository: mesos-git > > > Description > ------- > > Basically if you use "shell=false" with a non-empty argument list and the > Command Executor it is completely broken. > > When we clone the env vars to fork "mesos-executor" all of the original cmd > args are cloned as well (unintentionally) due to some protocol-buffer merge > shenanigans. > > Don't pass task-related arguments to mesos-executor. > > The description on the linked jira ticket goes into more detail. > > > Diffs > ----- > > src/slave/slave.cpp 0e342ed > src/tests/slave_tests.cpp f585bdd > > Diff: https://reviews.apache.org/r/26622/diff/ > > > Testing > ------- > > make check > > added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired > behavior. > > > Thanks, > > R.B. Boyer > >
