[
https://issues.apache.org/jira/browse/YARN-186?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13488769#comment-13488769
]
Robert Joseph Evans commented on YARN-186:
------------------------------------------
The tests look good. but I have a few comments.
# There are also two existing tests for the LinuxContainerExecutor.
TestLinuxContainerExecutor is more of an integration test and requires some
manual setup with the actual binary to make it work.
TestLinuxContainerExecutorWithMocks tries to test the code by mocking out parts
under it. The tests here look like they should be added into
TestLinuxContainerExecutorWithMocks instead of adding in a new file.
# In setup() a LinuxContainerExecutor exec is created, but it is only used in
one of the tests, testErrorLauncher. It would probably be better to have the
initialization happen in the test instead, which if you combine with
TestLinuxContainerExecutorWithMocks this becomes a bit of a noop, because it is
doing almost the exact same thing already for other tests.
# At a minimum some documentation about what writeScriptFile does would be
good. Having it write a script that will echo the arguments into a file named
after the last argument seems kind of confusing. Especially when to read those
arguments you need to open a file named '--checksetup'. I personally would like
to see it write the arguments out to a consistently named file, so that if the
arguments are wrong you get an assertion indicating that instead of a
FileNotFoundException. Which again if you combine with
TestLinuxContainerExecutorWithMocks the writeScriptFile goes away because there
is already code in the test to do something very similar.
# I would also like to see testInitException explained a little bit better what
it is trying to test. It is confusing why only setting the executor path to
/bin/sh causes init() to fail but in setup() you are setting it to /bin/bash,
and you skip calling the init() method.
Also if your patches are identical you don't really need to post separate
patches for each branch. One patch for trunk that applies to all of the
branches is simples to review.
> Coverage fixing LinuxContainerExecuror
> --------------------------------------
>
> Key: YARN-186
> URL: https://issues.apache.org/jira/browse/YARN-186
> Project: Hadoop YARN
> Issue Type: Test
> Components: resourcemanager, scheduler
> Reporter: Aleksey Gorshkov
> Attachments: YARN-186-branch-0.23--a.patch,
> YARN-186-branch-0.23.patch, YARN-186-branch-2--a.patch,
> YARN-186-branch-2.patch, YARN-186-trunk--a.patch, YARN-186-trunk.patch
>
>
> Added some tests for LinuxContainerExecuror
> YARN-186-branch-0.23.patch patch for branch-0.23
> YARN-186-branch-2.patch patch for branch-2
> ARN-186-trunk.patch patch for trank
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
