Copilot commented on code in PR #7875:
URL: https://github.com/apache/ignite-3/pull/7875#discussion_r2991318561
##########
modules/platforms/cpp/tests/odbc-test/transaction_test.cpp:
##########
@@ -657,5 +657,7 @@ TEST_F(transaction_test,
heartbeat_disable_connection_is_closed) {
ret = SQLEndTran(SQL_HANDLE_ENV, m_env, SQL_ROLLBACK);
- EXPECT_EQ(ret, SQL_ERROR);
+ std::cout << "Error message: " << get_odbc_error_message(SQL_HANDLE_ENV,
m_env) << std::endl;
+
+ EXPECT_TRUE(ret == SQL_ERROR || ret == SQL_SUCCESS_WITH_INFO);
Review Comment:
This test now treats `SQL_SUCCESS_WITH_INFO` as acceptable for `SQLEndTran`
without asserting any diagnostic state/message. Since `SQL_SUCCESS_WITH_INFO`
indicates the rollback succeeded (with warnings), the test no longer verifies
that the connection was actually closed. Consider either requiring `SQL_ERROR`,
or (if Windows legitimately returns SUCCESS_WITH_INFO here) asserting the
expected SQLSTATE/diagnostic content and/or adding a follow-up operation that
must fail due to closed connection.
```suggestion
EXPECT_EQ(SQL_ERROR, ret);
// Connection must be closed; subsequent operations are expected to fail.
EXPECT_THROW(insert_test_value(43, "Other"), odbc_exception);
```
##########
modules/platforms/cpp/tests/client-test/main.cpp:
##########
@@ -71,24 +71,28 @@ int main(int argc, char **argv) {
ignite_runner runner;
set_process_abort_handler([&](int signal) {
- std::cout << "Caught signal " << signal << " during tests" <<
std::endl;
+ std::cerr << "Caught signal " << signal << " during tests" <<
std::endl;
runner.stop();
});
if (!check_test_node_connectable(std::chrono::seconds(5))) {
runner.start();
- ensure_node_connectable(std::chrono::seconds(60));
+ auto timeout = std::chrono::minutes(5);
+ if (!check_test_node_connectable(timeout)) {
+ std::cerr << "Failed to start node within timeout: " <<
timeout.count() << "min" << std::endl;
+ return 3;
+ }
Review Comment:
`check_test_node_connectable(timeout)` uses
`ensure_node_connectable(timeout)` under the hood, which tries to connect to
*each* node address with the full timeout. In multi-node mode this can exceed
the intended 5-minute overall limit (and the error message will be misleading).
Consider enforcing an overall deadline (e.g., polling with `wait_for_condition`
and using a smaller per-attempt connect timeout) or dividing the timeout by the
number of endpoints.
##########
modules/platforms/cpp/tests/odbc-test/main.cpp:
##########
@@ -78,7 +78,11 @@ int main(int argc, char **argv) {
if (!check_test_node_connectable(std::chrono::seconds(5))) {
runner.start();
- ensure_node_connectable(std::chrono::seconds(60));
+ auto timeout = std::chrono::minutes(5);
+ if (!check_test_node_connectable(timeout)) {
+ std::cerr << "Failed to start node within timeout: " <<
timeout.count() << "min" << std::endl;
+ return 3;
+ }
Review Comment:
`check_test_node_connectable(timeout)` ultimately calls
`ensure_node_connectable(timeout)`, which attempts to connect to every
configured node with the full timeout. In multi-node mode that can exceed the
intended 5-minute overall limit (and the log message implies a hard 5-minute
deadline). Consider enforcing an overall deadline (e.g., `wait_for_condition`)
or splitting the timeout across endpoints.
##########
.teamcity/test/platform_tests/PlatformCppTestsWindows.kt:
##########
@@ -11,9 +12,10 @@ import
jetbrains.buildServer.configs.kotlin.failureConditions.failOnMetricChange
import jetbrains.buildServer.configs.kotlin.failureConditions.failOnText
import jetbrains.buildServer.configs.kotlin.triggers.vcs
import org.apache.ignite.teamcity.CustomBuildSteps.Companion.customGradle
-import org.apache.ignite.teamcity.CustomBuildSteps.Companion.customScript
+import org.apache.ignite.teamcity.CustomBuildSteps.Companion.customPowerShell
import org.apache.ignite.teamcity.Teamcity
import org.apache.ignite.teamcity.Teamcity.Companion.hiddenText
+import java.io.File
Review Comment:
The `java.io.File` import is unused in this file. Unused imports add noise
and may fail builds if the DSL compilation is configured to treat warnings as
errors.
```suggestion
```
##########
.teamcity/test/platform_tests/PlatformCppTestsWindows.kt:
##########
@@ -83,26 +82,66 @@ object PlatformCppTestsWindows : BuildType({
formatStderrAsError = true
}
customGradle {
- name = "Verify runner is builded"
+ name = "Verify runner is built"
tasks = ":ignite-runner:integrationTestClasses"
}
script {
name = "C++ Client integration tests"
workingDir = "%PATH__CMAKE_BUILD_DIRECTORY%"
- scriptContent = """Debug\bin\ignite-client-test
--gtest_output=xml:%PATH__CLIENT_TEST_RESULTS%"""
+ scriptContent = """
+ mkdir %PATH__CRASH_DUMPS% 2>nul
+ procdump -accepteula -ma -e -n 1 -x %PATH__CRASH_DUMPS%
Debug\bin\ignite-client-test --gtest_output=xml:%PATH__CLIENT_TEST_RESULTS%
+ if %%ERRORLEVEL%% NEQ 0 if %%ERRORLEVEL%% NEQ -2 (
+ echo procdump failed unexpectedly with code %%ERRORLEVEL%%
+ exit /b 1
+ )
Review Comment:
The procdump exit-code check is unlikely to work as intended: in Windows
CMD, a negative exit code like `-2` is typically reported via `%ERRORLEVEL%` as
an unsigned 32-bit value (e.g., `4294967294`), so `%%ERRORLEVEL%% NEQ -2` will
still be true and this step may fail even on the expected procdump outcome.
Consider checking for the unsigned value (or switching the wrapper to
PowerShell and comparing `$LASTEXITCODE` as an int), and make the accepted exit
codes explicit (e.g., allow both normal test exit and expected procdump codes).
##########
.teamcity/files/scripts/powershell/AnalyzeCrashDumps.ps1:
##########
@@ -0,0 +1,25 @@
+$dumpsDir = "%PATH__CRASH_DUMPS%"
+$binDir = "%PATH__CMAKE_BUILD_DIRECTORY%\Debug\bin"
+$cdb = "C:\Program Files (x86)\Windows Kits\10\Debuggers\x64\cdb.exe"
+$symPath = "$dumpsDir;$binDir"
+$srcPath = "%PATH__WORKING_DIR%"
+
Review Comment:
This script assumes `cdb.exe` exists at a hard-coded path. If the Windows
SDK Debuggers are missing or installed in a different location on an agent, the
script will fail with a fairly opaque PowerShell error. Consider adding an
explicit `Test-Path $cdb` check with a clear `Write-Error`/non-zero exit code
(or resolving the debugger path via environment variables/registry) so failures
are actionable in CI logs.
```suggestion
if (-not (Test-Path $cdb)) {
Write-Error "Crash dump analyzer requires cdb.exe, but it was not found
at the expected path: '$cdb'. Ensure the Windows SDK Debuggers are installed
and the path is correct."
exit 1
}
```
--
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]
