Bugzilla – Bug 4711
TCK JobOperatorTests Issues
Last modified: 2013-03-18 19:38:16 UTC
1. testJobOpGetJobNames - The number of names returned isn't verified.
2. testJobOpgetJobInstanceCount - Should the job fail if a NoSuchJobException is *not* thrown? Seems non-deterministic…
3. testJobOpgetJobInstanceCountException - Test does not fail if the exception at the end is *not* thrown.
4. testJobOpgetJobInstances - The assertWithMessage in the bottom try/catch is confusing…you are catching a NoSuchJobException…yet expect the exception to not be an instance of it (probably should be catching Exception)? Also, there is no validation that the exception did (or did not) occur. Finally, I would expect the size of the jobInstances list to be determinant (not just > 0).
5. testJobOpgetJobInstancesException - While the assert within the catch is redundant, there is also no validation that the exception is actually thrown (what happens if it's not)?
6. testJobOperatorGetParameters - This test uses the JobInstance to get the parameters…the spec says it should be the JobExecution. The parameters returned are never validated.
7. testJobOperatorGetJobInstance - The JobInstance returned is not validated in any way other than it was the correct type (which would have thrown a ClassCastException anyways).
8. testJobOperatorGetRunningJobInstances - Why is this checking > 0 instead of 2?
9. testJobOperatorGetRunningJobInstancesExeception - Does not validate the exception was thrown…just that if it was, it was the correct one.
10. testJobOperatorGetJobExecution - Does not validate any of the data within the returned JobExecution.
Thanks for the review, and sorry for just getting around to it (but I was just fixing something else in this class so got back to your comments).
You made some good points... and actually there were a couple other things we didn't test like JobExecutionAlreadyCompleteException, JobExecutionNotMostRecentException too.
To your points (the fixed one should be out tomorrow):
1. Fixed (or improved, see 2. below).
2. It's a good question, but no it should not. The reason is we don't require a clean job repository to run the TCK. I don't think it's important enough to force that. We don't have a "cleanup from TCK" routine of any form..... so it's better this way.
4. Fixed. Made me realize an unspec'd detail.. if you ask for 12 and there are only 10, it's up to the implementation to decide what to do...
7. I can't find this method.. did some cleanup here including fixing all the "instanceof' checks and making sure we were verifying that we had seen exceptions and failing if we hadn't... you'll have to let me know if you still see an issue
8. It's just a tradeoff between being more strict and avoiding "false positives" by failing due to a timing window when there's nothing wrong with the runtime. We've got some buffer built in, true...but say you forgot to implement the API.. you'd fail then so the test does serve a purpose.
10. Maybe we don't even need this test... we do plenty of looking at JobExecution(s).
One other note, I changed some of these from blocking to non-blocking, i.e. look at instance count immediately.
With regards to number 8, this test has changed a bit now and I'm not sure how it accomplishes the goal of validating the JobOperator#getRunningJobInstances. Now it would pass if the jobOp.getRunningExecutions("JOBNAMEDOESNOTEXIST"); call throws an exception, but won't fail if it doesn't. Also, because this method now is just checking for that exception, running any jobs before the call is pointless.
I'm marking this fixed... we also fixed #8 to ONLY pass if the execption is caught.