----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13674/#review26138 -----------------------------------------------------------
Review comments till TestEventGeneration (Page 1 and part of page 2). Will complete the rest by end of day. http://svn.apache.org/repos/asf/oozie/trunk/core/src/main/java/org/apache/oozie/service/ConfigurationService.java <https://reviews.apache.org/r/13674/#comment51020> getPath() http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/coord/TestCoordActionInputCheckXCommand.java <https://reviews.apache.org/r/13674/#comment51021> Can you change the createDir method and make it take File as an argument instead of doing getAbsolutePath every time. Just to make writing easier as this is repeatedly done. http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/coord/TestCoordActionStartXCommand.java <https://reviews.apache.org/r/13674/#comment51022> Why wfAppPath.toString() and not just wfAppPath? http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/wf/TestSubmitXCommand.java <https://reviews.apache.org/r/13674/#comment51028> Why not getTestCaseFileUri in both places? Would be better to move that out as a variable. Confusing to see one new File(getTestCaseDir(),..) and another getTestCaseFileUri Same for other methods in this class. http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/coord/TestCoordELEvaluator.java <https://reviews.apache.org/r/13674/#comment51037> You can pass File or a Path as argument to createDir instead of doing toString. Also getPath or getAbsolutePath is better instead of toString and you have already used that in most places except for some cases like this. http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/coord/TestCoordELFunctions.java <https://reviews.apache.org/r/13674/#comment51042> Same as before. Can pass File. We should move this method to XTestCase as it is getting repeated in lot of testcases. http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/test/XTestCase.java <https://reviews.apache.org/r/13674/#comment51030> Wouldn't File.Separator be better instead of "/" ? http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/test/XTestCase.java <https://reviews.apache.org/r/13674/#comment51033> Formatting. Spaces before and after operators like = < + - Rohini Palaniswamy On Aug. 19, 2013, 10:31 p.m., David Wannemacher wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13674/ > ----------------------------------------------------------- > > (Updated Aug. 19, 2013, 10:31 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-1500 > https://issues.apache.org/jira/browse/OOZIE-1500 > > > Repository: oozie > > > Description > ------- > > This is a large patch, please let me know if I should split it up and/or use > the code reviewer tool. I tested it on linux and verified that it did not > cause any unit test regressions. > > This patch fixes many unix-specific assumptions in code and tests: > 1. Using "/" as a path separator. On windows, the right separator is "\". > Fix: create a new Path object with the directory and subdirectory names, > which handles using the right separator. > 2. Determining whether a path is absolute by testing if it begins with "/". > Fix: Create a new File object and use isAbsolute(). > 3. Generating a pathname by concatenating "file://", getTestCaseDir(), > File.separator, and the file name. This is wrong because this is a URI, and > File.separator is "/" on unix but "\" on windows. Fix: New helper method > getTestCaseFileUri solves this. > 4. Create directories by using "mkdir" shell command with hard-coded file > separators. Fix: Create a new File object and use mkdirs(). > 5. Scripts that use unix-only commands (ls, echo, etc.). Fix: Implement > windows-specific commands and use Shell.WINDOWS to pick the right one at > runtime. > 6. Assume that perl exists in TestShellActionExecutor.testPerlScript. It > doesn't on Windows. Fix: skip the test on Windows. > 7. Use the test case name as part of the temp directory name in order to > ensure directories are unique. This can cause issues on Windows. Fix: Use a > random UUID instead. > 8. As a result of using getTestCaseUri, some test case verification that uses > string equality fails. (TestLiteWorkflowAppService) Fix: use URI equivalence > instead of string equality. > 9. AdminServlet tests verify that system variables are working by checking > for a USER variable. This is called USERNAME in windows. Fix: On Windows, > check for USERNAME instead. > 10. TestHostnameFilter checks that "localhost" is in the filter. On Windows, > this is "127.0.0.1". Fix: On Windows, check for 127.0.0.1 instead. > > On windows there are still several tests failing. I hope to spend some time > fixing these later: > Failed tests: > testActionCheck(org.apache.oozie.command.wf.TestActionCheckXCommand) > testActionCheckTransientDuringLauncher(org.apache.oozie.command.wf.TestActionCheckXCommand) > testActionCheckTransientDuringMRAction(org.apache.oozie.command.wf.TestActionCheckXCommand) > testCoordinatorActionCommands(org.apache.oozie.sla.TestSLAEventGeneration): > expected:<NOT_STARTED> but was:<IN_PROCESS> > testChmodWithGlob(org.apache.oozie.action.hadoop.TestFsActionExecutor): > expected:<rw[-]------> but was:<rw[x]------> > Tests in error: > testWaitFor(org.apache.oozie.test.TestXTestCase): could not delete path > [D:\apache\oozie\core\target\test-data\oozietests\org.apache.oozie.test.TestXTestCase$MyXTestCase\testWaitFor\conf\oozie-site.xml] > testBaseDir(org.apache.oozie.test.TestXTestCase): could not delete path > [D:\apache\oozie\core\target\test-data\oozietests\org.apache.oozie.test.TestXTestCase$MyXTestCase\testBaseDir\conf\oozie-site.xml] > > How to run unit tests in windows: > 1. You will need to build hadoop-core from branch-1-win and install it into > your local maven cache. > 2. In the root pom.xml, replace the dummy value in HADOOP_HOME with the path > to the hadoop directory. The minicluster tests require bin\winutils.exe to be > in this directory. > 3. In the root pom.xml (and hadooplibs version 1 pom.xmls, if OOZIE-1490 is > not committed yet) change the hadoop version to the version built from > branch-1-win. > 4. May also need to use Hive 0.11 with the windows fixes. > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/main/java/org/apache/oozie/service/CallbackService.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/main/java/org/apache/oozie/service/ConfigurationService.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/main/java/org/apache/oozie/service/Services.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/TestCoordinatorEngine.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/TestCoordinatorEngineStreamLog.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/TestDagEngine.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/TestLocalOozieClientCoord.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/action/hadoop/ShellTestCase.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestRerun.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestShellActionExecutor.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/action/hadoop/TestShellMain.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/action/oozie/TestSubWorkflowActionExecutor.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/client/TestLocalOozie.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/coord/TestCoordActionInputCheckXCommand.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/coord/TestCoordActionStartXCommand.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/coord/TestCoordSubmitXCommand.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/coord/TestFutureActionsTimeOut.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/coord/TestPastActionsTimeOut.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/wf/TestActionErrors.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/wf/TestLastModified.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/wf/TestReRunXCommand.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/wf/TestSignalXCommand.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/command/wf/TestSubmitXCommand.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/coord/TestCoordELEvaluator.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/coord/TestCoordELFunctions.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/event/TestEventGeneration.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordActionGetForInputCheckJPAExecutor.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetActionsSubsetJPAExecutor.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestActionCheckerService.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestAuthorizationService.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestLiteWorkflowAppService.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestPurgeService.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/service/TestRecoveryService.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/servlet/TestAdminServlet.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/servlet/TestHostnameFilter.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/servlet/TestV1AdminServlet.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/test/TestXFsTestCase.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/test/XDataTestCase.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/test/XFsTestCase.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/test/XHCatTestCase.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/core/src/test/java/org/apache/oozie/test/XTestCase.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/examples/src/main/java/org/apache/oozie/example/LocalOozieExample.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/minitest/src/test/java/org/apache/oozie/test/WorkflowTest.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/sharelib/hive/src/main/java/org/apache/oozie/action/hadoop/HiveMain.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherMain.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/MapReduceMain.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PipesMain.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/sharelib/pig/src/main/java/org/apache/oozie/action/hadoop/PigMain.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/sharelib/pig/src/main/java/org/apache/oozie/action/hadoop/PigMainWithOldAPI.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/sharelib/sqoop/src/main/java/org/apache/oozie/action/hadoop/SqoopMain.java > 1515521 > > http://svn.apache.org/repos/asf/oozie/trunk/sharelib/streaming/src/main/java/org/apache/oozie/action/hadoop/StreamingMain.java > 1515521 > > Diff: https://reviews.apache.org/r/13674/diff/ > > > Testing > ------- > > Ran unit tests on Windows and Linux. > > > Thanks, > > David Wannemacher > >
