> On Jan. 24, 2017, 6:14 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java, > > lines 572-586 > > <https://reviews.apache.org/r/51029/diff/3-4/?file=1612950#file1612950line572> > > > > For all similar `try-catch` stories like this, beginning w/ JUnit 4.7: > > > > ``` > > @Rule > > public ExpectedException expected = ExpectedException.none(); > > ... > > expected.expect(MyOoziException.class); > > expected.expectMessage(myExceptionMessageString); > > performAndThrow(); > > ``` > > Satish Saley wrote: > The exception that we need to check is a **wrapped exception**. We can > check the exception with expected.expectCause from junit 4.11; but we cannot > check the exception message string. Dropping this. > > András Piros wrote: > You could just define a custom `OozieCauseMatcher extends > TypeSafeMatcher<Throwable>` as per: > > > https://www.javacodegeeks.com/2014/03/junit-expectedexception-rule-beyond-basics.html > > but at least refactor to a method that does the > `try-catch-assertCauseMessage` stuff for you and reuse that multiple times.
Thank you for the reference. I tried it, but i am unable to get it working. I think as long as test is serving its purpose, it should be ok to go with try-catch block. Also, if we go with ExpectedException, we need to make changes on junit 5. > On Jan. 24, 2017, 6:14 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java, > > lines 591-606 > > <https://reviews.apache.org/r/51029/diff/3-4/?file=1612950#file1612950line591> > > > > For all similar `try-catch` stories like this, beginning w/ JUnit 4.7: > > > > ``` > > @Rule > > public ExpectedException expected = ExpectedException.none(); > > ... > > expected.expect(MyOoziException.class); > > expected.expectMessage(myExceptionMessageString); > > performAndThrow(); > > ``` > > Satish Saley wrote: > The exception that we need to check is a **wrapped exception**. We can > check the exception with expected.expectCause from junit 4.11; but we cannot > check the exception message string. Dropping this. > > András Piros wrote: > You could just define a custom `OozieCauseMatcher extends > TypeSafeMatcher<Throwable>` as per: > > > https://www.javacodegeeks.com/2014/03/junit-expectedexception-rule-beyond-basics.html > > but at least refactor to a method that does the > `try-catch-assertCauseMessage` stuff for you and reuse that multiple times. Thank you for the reference. I spent couple of hours on it, but i am unable to get it working. I think as long as test is serving its purpose, it should be ok to go with try-catch block. Also, if we go with ExpectedException, we need to make changes on junit 5. > On Jan. 24, 2017, 6:14 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java, > > lines 610-626 > > <https://reviews.apache.org/r/51029/diff/3-4/?file=1612950#file1612950line610> > > > > For all similar `try-catch` stories like this, beginning w/ JUnit 4.7: > > > > ``` > > @Rule > > public ExpectedException expected = ExpectedException.none(); > > ... > > expected.expect(MyOoziException.class); > > expected.expectMessage(myExceptionMessageString); > > performAndThrow(); > > ``` > > Satish Saley wrote: > Here I can use ExpectedException since exception is not wrapped. But as i > mentioned in my earlier comments. ExpectedException is going to be removed > from junit sooner. So I would rather stick to the try-catch. > > András Piros wrote: > You could just define a custom `OozieCauseMatcher extends > TypeSafeMatcher<Throwable>` as per: > > > https://www.javacodegeeks.com/2014/03/junit-expectedexception-rule-beyond-basics.html > > but at least refactor to a method that does the > `try-catch-assertCauseMessage` stuff for you and reuse that multiple times. Thank you for the reference. I spent couple of hours on it, but i am unable to get it working. I think as long as test is serving its purpose, it should be ok to go with try-catch block. Also, if we go with ExpectedException, we need to make changes on junit 5. > On Jan. 24, 2017, 6:14 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java, > > lines 683-697 > > <https://reviews.apache.org/r/51029/diff/3-4/?file=1612950#file1612950line683> > > > > For all similar `try-catch` stories like this, beginning w/ JUnit 4.7: > > > > ``` > > @Rule > > public ExpectedException expected = ExpectedException.none(); > > ... > > expected.expect(MyOoziException.class); > > expected.expectMessage(myExceptionMessageString); > > performAndThrow(); > > ``` > > Satish Saley wrote: > The exception that we need to check is a **wrapped exception**. We can > check the exception with expected.expectCause from junit 4.11; but we cannot > check the exception message string. Dropping this. > > András Piros wrote: > You could just define a custom `OozieCauseMatcher extends > TypeSafeMatcher<Throwable>` as per: > > > https://www.javacodegeeks.com/2014/03/junit-expectedexception-rule-beyond-basics.html > > but at least refactor to a method that does the > `try-catch-assertCauseMessage` stuff for you and reuse that multiple times. Thank you for the reference. I spent couple of hours on it, but i am unable to get it working. I think as long as test is serving its purpose, it should be ok to go with try-catch block. Also, if we go with ExpectedException, we need to make changes on junit 5. > On Jan. 24, 2017, 6:14 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java, > > lines 702-717 > > <https://reviews.apache.org/r/51029/diff/3-4/?file=1612950#file1612950line702> > > > > For all similar `try-catch` stories like this, beginning w/ JUnit 4.7: > > > > ``` > > @Rule > > public ExpectedException expected = ExpectedException.none(); > > ... > > expected.expect(MyOoziException.class); > > expected.expectMessage(myExceptionMessageString); > > performAndThrow(); > > ``` > > Satish Saley wrote: > The exception that we need to check is a **wrapped exception**. We can > check the exception with expected.expectCause from junit 4.11; but we cannot > check the exception message string. Dropping this. > > András Piros wrote: > You could just define a custom `OozieCauseMatcher extends > TypeSafeMatcher<Throwable>` as per: > > > https://www.javacodegeeks.com/2014/03/junit-expectedexception-rule-beyond-basics.html > > but at least refactor to a method that does the > `try-catch-assertCauseMessage` stuff for you and reuse that multiple times. Thank you for the reference. I spent couple of hours on it, but i am unable to get it working. I think as long as test is serving its purpose, it should be ok to go with try-catch block. Also, if we go with ExpectedException, we need to make changes on junit 5. > On Jan. 24, 2017, 6:14 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java, > > lines 721-736 > > <https://reviews.apache.org/r/51029/diff/3-4/?file=1612950#file1612950line721> > > > > For all similar `try-catch` stories like this, beginning w/ JUnit 4.7: > > > > ``` > > @Rule > > public ExpectedException expected = ExpectedException.none(); > > ... > > expected.expect(MyOoziException.class); > > expected.expectMessage(myExceptionMessageString); > > performAndThrow(); > > ``` > > Satish Saley wrote: > Here I can use ExpectedException since exception is not wrapped. But as i > mentioned in my earlier comments. ExpectedException is going to be removed > from junit sooner. So I would rather stick to the try-catch. > > András Piros wrote: > You could just define a custom `OozieCauseMatcher extends > TypeSafeMatcher<Throwable>` as per: > > > https://www.javacodegeeks.com/2014/03/junit-expectedexception-rule-beyond-basics.html > > but at least refactor to a method that does the > `try-catch-assertCauseMessage` stuff for you and reuse that multiple times. Thank you for the reference. I spent couple of hours on it, but i am unable to get it working. I think as long as test is serving its purpose, it should be ok to go with try-catch block. Also, if we go with ExpectedException, we need to make changes on junit 5. > On Jan. 24, 2017, 6:14 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java, > > lines 802-817 > > <https://reviews.apache.org/r/51029/diff/3-4/?file=1612950#file1612950line802> > > > > For all similar `try-catch` stories like this, beginning w/ JUnit 4.7: > > > > ``` > > @Rule > > public ExpectedException expected = ExpectedException.none(); > > ... > > expected.expect(MyOoziException.class); > > expected.expectMessage(myExceptionMessageString); > > performAndThrow(); > > ``` > > Satish Saley wrote: > The exception that we need to check is a **wrapped exception**. We can > check the exception with expected.expectCause from junit 4.11; but we cannot > check the exception message string. Dropping this. > > András Piros wrote: > You could just define a custom `OozieCauseMatcher extends > TypeSafeMatcher<Throwable>` as per: > > > https://www.javacodegeeks.com/2014/03/junit-expectedexception-rule-beyond-basics.html > > but at least refactor to a method that does the > `try-catch-assertCauseMessage` stuff for you and reuse that multiple times. Thank you for the reference. I spent couple of hours on it, but i am unable to get it working. I think as long as test is serving its purpose, it should be ok to go with try-catch block. Also, if we go with ExpectedException, we need to make changes on junit 5. > On Jan. 24, 2017, 6:14 p.m., András Piros wrote: > > core/src/test/java/org/apache/oozie/command/coord/TestCoordCommandUtils.java, > > lines 838-854 > > <https://reviews.apache.org/r/51029/diff/3-4/?file=1612950#file1612950line838> > > > > For all similar `try-catch` stories like this, beginning w/ JUnit 4.7: > > > > ``` > > @Rule > > public ExpectedException expected = ExpectedException.none(); > > ... > > expected.expect(MyOoziException.class); > > expected.expectMessage(myExceptionMessageString); > > performAndThrow(); > > ``` > > Satish Saley wrote: > Here I can use ExpectedException since exception is not wrapped. But as i > mentioned in my earlier comments. ExpectedException is going to be removed > from junit sooner. So I would rather stick to the try-catch. > > András Piros wrote: > You could just define a custom `OozieCauseMatcher extends > TypeSafeMatcher<Throwable>` as per: > > > https://www.javacodegeeks.com/2014/03/junit-expectedexception-rule-beyond-basics.html > > but at least refactor to a method that does the > `try-catch-assertCauseMessage` stuff for you and reuse that multiple times. Thank you for the reference. I spent couple of hours on it, but i am unable to get it working. I think as long as test is serving its purpose, it should be ok to go with try-catch block. Also, if we go with ExpectedException, we need to make changes on junit 5. - Satish ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51029/#review162866 ----------------------------------------------------------- On Jan. 24, 2017, 2:49 p.m., Satish Saley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51029/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2017, 2:49 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 > >
