----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51029/#review162674 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java (line 283) <https://reviews.apache.org/r/51029/#comment233994> Please use `DAY_OF_MONTH` instead and externalize `1` to `FIRST_WEEK_OF_MONTH` for better readability. core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java (line 302) <https://reviews.apache.org/r/51029/#comment233996> Calendars like European ones do not necessarily have Monday as the last day of week - use `calendar.getFirstDayOfWeek()` instead. core/src/main/java/org/apache/oozie/command/coord/CoordSubmitXCommand.java (line 747) <https://reviews.apache.org/r/51029/#comment233998> Are you sure each and every month consists of exactly 30 days? I think it's a bug - I know not in the lines of code you touched, but indeed, maybe worth fixing it. core/src/main/java/org/apache/oozie/util/DateUtils.java (line 307) <https://reviews.apache.org/r/51029/#comment233999> Calendars like European ones do not necessarily have Monday as the last day of week - use `calendar.getFirstDayOfWeek()` instead. core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (line 521) <https://reviews.apache.org/r/51029/#comment234000> You perform too much testing in one single test method. I'd have as many methods as testing use cases with appropriate naming strategy like http://osherove.com/blog/2005/4/3/naming-standards-for-unit-tests.html so that for every unit test we exactly know what it's testing and what is the expected behavior without needing to read the code, just reading the `Exception` logs. core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (line 532) <https://reviews.apache.org/r/51029/#comment234001> Extract `DateUtils.parseDateOozieTZ("2009-08-20T01:00Z")` to a well-named variable. core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (line 543) <https://reviews.apache.org/r/51029/#comment234003> Extract `DateUtils.parseDateOozieTZ("2009-08-01T01:00Z")` to a well-named variable. core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (line 549) <https://reviews.apache.org/r/51029/#comment234004> Extract `DateUtils.parseDateOozieTZ("2009-08-01T01:00Z")` to a well-named variable. core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (line 557) <https://reviews.apache.org/r/51029/#comment234005> Extract `DateUtils.parseDateOozieTZ("2009-08-01T01:00Z")` to a well-named variable. core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (line 566) <https://reviews.apache.org/r/51029/#comment234006> Extract `DateUtils.parseDateOozieTZ("2009-07-01T01:00Z")` to a well-named variable. core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (line 575) <https://reviews.apache.org/r/51029/#comment234007> Extract `DateUtils.parseDateOozieTZ()` to a well-named variable. core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (lines 580 - 583) <https://reviews.apache.org/r/51029/#comment234008> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (lines 596 - 599) <https://reviews.apache.org/r/51029/#comment234009> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (lines 613 - 617) <https://reviews.apache.org/r/51029/#comment234010> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (lines 679 - 682) <https://reviews.apache.org/r/51029/#comment234011> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (lines 695 - 698) <https://reviews.apache.org/r/51029/#comment234015> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (line 710) <https://reviews.apache.org/r/51029/#comment234016> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (lines 712 - 715) <https://reviews.apache.org/r/51029/#comment234017> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (line 719) <https://reviews.apache.org/r/51029/#comment234019> You perform too much testing in one single test method. I'd have as many methods as testing use cases with appropriate naming strategy like http://osherove.com/blog/2005/4/3/naming-standards-for-unit-tests.html so that for every unit test we exactly know what it's testing and what is the expected behavior without needing to read the code, just reading the `Exception` logs. core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (line 779) <https://reviews.apache.org/r/51029/#comment234020> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (lines 781 - 784) <https://reviews.apache.org/r/51029/#comment234021> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (line 795) <https://reviews.apache.org/r/51029/#comment234022> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (lines 797 - 800) <https://reviews.apache.org/r/51029/#comment234023> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (line 812) <https://reviews.apache.org/r/51029/#comment234024> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java (lines 814 - 817) <https://reviews.apache.org/r/51029/#comment234025> Use JUnit's `ExpectedException` instead: https://github.com/junit-team/junit4/wiki/exception-testing core/src/test/java/org/apache/oozie/command/coord/TestCoordMaterializeTransitionXCommand.java (line 746) <https://reviews.apache.org/r/51029/#comment234018> You perform too much testing in one single test method. I'd have as many methods as testing use cases with appropriate naming strategy like http://osherove.com/blog/2005/4/3/naming-standards-for-unit-tests.html so that for every unit test we exactly know what it's testing and what is the expected behavior without needing to read the code, just reading the `Exception` logs. - András Piros On Jan. 23, 2017, 5:22 p.m., Satish Saley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51029/ > ----------------------------------------------------------- > > (Updated Jan. 23, 2017, 5:22 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-2630 > https://issues.apache.org/jira/browse/OOZIE-2630 > > > Repository: oozie-git > > > Description > ------- > > Oozie Coordinator EL Functions to get first day of the week/month > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/command/coord/CoordCommandUtils.java > 0af7edc > core/src/main/java/org/apache/oozie/command/coord/CoordSubmitXCommand.java > 969336d > core/src/main/java/org/apache/oozie/coord/CoordELFunctions.java 925a7aa > core/src/main/java/org/apache/oozie/coord/TimeUnit.java 5b37639 > core/src/main/java/org/apache/oozie/util/DateUtils.java 3caf0a2 > core/src/main/resources/oozie-default.xml ad10386 > > core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java > 7062e69 > > core/src/test/java/org/apache/oozie/command/coord/TestCoordMaterializeTransitionXCommand.java > 29e7ca1 > core/src/test/java/org/apache/oozie/coord/TestCoordELFunctions.java be60133 > core/src/test/resources/coord-dataset-endOfDays.xml PRE-CREATION > core/src/test/resources/coord-dataset-endOfMonths.xml PRE-CREATION > core/src/test/resources/coord-dataset-endOfWeeks.xml PRE-CREATION > docs/src/site/twiki/CoordinatorFunctionalSpec.twiki e3ac514 > > Diff: https://reviews.apache.org/r/51029/diff/ > > > Testing > ------- > > Tested locally > > > Thanks, > > Satish Saley > >
