lhotari commented on code in PR #23811:
URL: https://github.com/apache/pulsar/pull/23811#discussion_r1906727569
##########
pulsar-functions/instance/src/test/java/org/apache/pulsar/functions/instance/JavaInstanceTest.java:
##########
@@ -309,4 +317,25 @@ public void testAsyncFunctionMaxPendingVoidResult() throws
Exception {
log.info("start:{} end:{} during:{}", startTime, endTime, endTime -
startTime);
instance.close();
}
+
+ @Test
+ public void testAsyncFunctionTime() {
+ JavaInstance instance = new JavaInstance(
+ mock(ContextImpl.class),
+ (Function<String, String>) (input, context) -> {
+ try {
+ Thread.sleep(500);
+ } catch (InterruptedException e) {
+ e.printStackTrace();
+ }
+ return input;
+ },
+ new InstanceConfig());
+ String testString = "ABC123";
+ JavaExecutionResult result =
instance.handleMessage(mock(Record.class), testString);
+ assertNotNull(result.getResult());
+ long beforeTime = System.nanoTime() - 500_000_000L;
+ assertTrue(Math.abs(beforeTime - result.getStartTime()) <= 20_000_000);
Review Comment:
It seems pointless to test that at all in this unit test class. It would
essentially test that `result.getStartTime()` was set before the execution
started and that execution took more than 500ms. Tests should be about testing
expected behavior.
In this PR, we are more interested that the processing time gets properly
recorded. That would have to be tested at a different level since
JavaInstanceTest is at the level of the JavaInstance class. The processing time
handling is in JavaInstanceRunnable. I think that tests for this PR should
reside in JavaInstanceRunnable and possibly also in one of the end-to-end tests
(for example
`pulsar-broker/src/test/java/org/apache/pulsar/io/PulsarFunctionE2ETest.java`).
btw. One tip about assertions. For newly added tests, I'd recommend using
[AssertJ fluent assertions](https://assertj.github.io/doc/) for more complex
assertions. We have AssertJ already available as a test dependency in Pulsar.
It's not that I'm saying that AssertJ must be used, it's just that if there are
more complex assertions, it's a better option than TestNG's assertions. AssertJ
assertions are easier to read and when there are failures, the error messages
are really great.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
