----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64663/#review194046 -----------------------------------------------------------
core/src/test/java/org/apache/oozie/action/hadoop/ShellTestCase.java Lines 67-81 (patched) <https://reviews.apache.org/r/64663/#comment272758> Let's take into separate test class `TestShellContentWriter`, and have different and well-named test cases for each scenario. Apart from this, what about more elaborate test cases like: * too long scripts * scripts with 0 bytes * files containing not only `0x00` but `0x09` as well core/src/test/java/org/apache/oozie/action/hadoop/ShellTestCase.java Lines 68-69 (patched) <https://reviews.apache.org/r/64663/#comment272759> What about using JUnit 4's `TemporaryFolder`? core/src/test/java/org/apache/oozie/action/hadoop/ShellTestCase.java Lines 84-94 (patched) <https://reviews.apache.org/r/64663/#comment272757> If you have `ShellContentWriter` and inject an `OutputStream` via constructor, you'll be able to test it with a mock `OutputStream` without the need of setting and resetting `System.out` or `System.err`. core/src/test/java/org/apache/oozie/action/hadoop/ShellTestCase.java Lines 86 (patched) <https://reviews.apache.org/r/64663/#comment272756> Use preconfigured value here. sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java Lines 55 (patched) <https://reviews.apache.org/r/64663/#comment272747> Let's call this constant something like `MAX_SCRIPT_SIZE_TO_PRINT_KB`, and make it configurable via `ConfigurationService`. Have a sane default value within `oozie-default.xml`, too. sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java Lines 331 (patched) <https://reviews.apache.org/r/64663/#comment272749> Some sanity checks on `cmdArray` wouldn't harm. sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java Lines 333-334 (original), 348-386 (patched) <https://reviews.apache.org/r/64663/#comment272750> Extract to - at least nested - class `ShellContentWriter` that takes `OutputStream` and `fileName` as parameters. You can then conveniently unit test that. sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java Lines 357 (patched) <https://reviews.apache.org/r/64663/#comment272752> What about files with 0 bytes? sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java Lines 358 (patched) <https://reviews.apache.org/r/64663/#comment272751> I wouldn't read all the bytes, but read 1-by-1, and check whether the actual one is under `0x09`. If so, it's handled as a binary file, else as a shell script. sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java Lines 370 (patched) <https://reviews.apache.org/r/64663/#comment272753> Maybe `System.err.println()` the error messages. sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java Lines 373-375 (patched) <https://reviews.apache.org/r/64663/#comment272754> `catch (final IOException ignored) {}` and then you don't need to comment anything. sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java Line 334 (original), 379-386 (patched) <https://reviews.apache.org/r/64663/#comment272755> I wouldn't read all the bytes, but read 1-by-1, and check whether the actual one is under `0x09`. If so, it's handled as a binary file, else as a shell script. Let's not make this one static, and part of `ShellContentWriter`. - András Piros On Dec. 16, 2017, 12:18 a.m., Jacob Tolar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64663/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2017, 12:18 a.m.) > > > Review request for oozie. > > > Repository: oozie-git > > > Description > ------- > > [OOZIE-2150] Shell launcher should print shell script > > Oozie shell launcher will dump script into log. The script is printed if it > is (1) not too big and (2) appears to be plain-text (no null bytes). > > > Diffs > ----- > > core/src/test/java/org/apache/oozie/action/hadoop/ShellTestCase.java > 1a654f72 > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java > 55d3d965 > > > Diff: https://reviews.apache.org/r/64663/diff/1/ > > > Testing > ------- > > unit test added > > > Thanks, > > Jacob Tolar > >
