Copilot commented on code in PR #16898:
URL: https://github.com/apache/iotdb/pull/16898#discussion_r2610273820
##########
integration-test/src/test/java/org/apache/iotdb/ainode/utils/AINodeTestUtils.java:
##########
@@ -113,13 +116,17 @@ public static void concurrentInference(
while (resultSet.next()) {
outputCnt++;
}
- assertEquals(expectedOutputLength, outputCnt);
+ if (expectedOutputLength != outputCnt) {
+ allPass.set(false);
Review Comment:
When the output count doesn't match the expected length, the test should
fail immediately with a descriptive error message. Setting the flag without
providing any diagnostic information makes it harder to debug test failures.
Consider logging the actual and expected values before setting the flag to
false.
```suggestion
fail("Output count mismatch for SQL: " + sql + ".
Expected: " + expectedOutputLength + ", but got: " + outputCnt);
```
##########
integration-test/src/test/java/org/apache/iotdb/ainode/utils/AINodeTestUtils.java:
##########
@@ -113,13 +116,17 @@ public static void concurrentInference(
while (resultSet.next()) {
outputCnt++;
}
- assertEquals(expectedOutputLength, outputCnt);
+ if (expectedOutputLength != outputCnt) {
+ allPass.set(false);
+ }
} catch (SQLException e) {
fail(e.getMessage());
+ allPass.set(false);
}
}
} catch (Exception e) {
fail(e.getMessage());
Review Comment:
Calling fail() from within a worker thread will not fail the test as
expected. The fail() method throws an AssertionError, but since this is inside
a thread's run method, it will only terminate that specific thread without
failing the overall test. The allPass flag is correctly set to false, but the
fail() call here is ineffective and misleading.
```suggestion
allPass.set(false);
}
}
} catch (Exception e) {
```
##########
integration-test/src/test/java/org/apache/iotdb/ainode/utils/AINodeTestUtils.java:
##########
@@ -129,6 +136,7 @@ public static void concurrentInference(
if (thread.isAlive()) {
fail("Thread timeout after 10 minutes");
}
+ Assert.assertTrue(allPass.get());
}
Review Comment:
The assertion is placed inside the loop and checks allPass after each thread
joins. This means if any thread fails, all subsequent iterations of the loop
will also fail the assertion. The assertion should be moved outside the loop to
check the final state only once after all threads have completed.
```suggestion
}
Assert.assertTrue(allPass.get());
```
##########
integration-test/src/test/java/org/apache/iotdb/ainode/utils/AINodeTestUtils.java:
##########
@@ -113,13 +116,17 @@ public static void concurrentInference(
while (resultSet.next()) {
outputCnt++;
}
- assertEquals(expectedOutputLength, outputCnt);
+ if (expectedOutputLength != outputCnt) {
+ allPass.set(false);
+ }
} catch (SQLException e) {
fail(e.getMessage());
+ allPass.set(false);
}
}
} catch (Exception e) {
fail(e.getMessage());
Review Comment:
Calling fail() from within a worker thread will not fail the test as
expected. The fail() method throws an AssertionError, but since this is inside
a thread's run method, it will only terminate that specific thread without
failing the overall test. The allPass flag is correctly set to false, but the
fail() call here is ineffective and misleading.
```suggestion
allPass.set(false);
}
}
} catch (Exception e) {
```
--
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]
