Copilot commented on code in PR #6856: URL: https://github.com/apache/ignite-3/pull/6856#discussion_r2469411017
########## modules/platforms/cpp/tests/compatibility-tests/main.cpp: ########## @@ -0,0 +1,236 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +#include "ignite_runner.h" +#include "ignite_xml_unit_test_result_printer.h" +#include "test_utils.h" + +#include <gtest/gtest.h> + +#include <chrono> +#include <csignal> +#include <cstring> + +namespace { + +/** Shutdown handler that cleans up resources. */ +std::function<void(int)> shutdown_handler; + +/** + * Receives OS signal and handles it. + * + * @param signum Signal value. + */ +void signal_handler(int signum) { + shutdown_handler(signum); + signal(signum, SIG_DFL); + raise(signum); +} + +} // namespace + +/** + * Sets process abortion (SIGABRT, SIGINT, SIGSEGV signals) handler. + * + * @param handler Abortion handler. + */ +void set_process_abort_handler(std::function<void(int)> handler) { + shutdown_handler = std::move(handler); + + // Install signal handlers to clean up resources on early exit. + signal(SIGABRT, signal_handler); + signal(SIGINT, signal_handler); + signal(SIGSEGV, signal_handler); +} + +using namespace ignite; + +const std::vector<std::string> DEFAULT_VERSIONS = {"9.1.0", "9.1.1", "9.1.2", "9.1.3", "9.1.4"}; + +/** + * Structure to store argument values for automatic memory management. + */ +struct ArgumentValuesHolder { +private: + int m_argc; + std::vector<std::string> m_vals; + char **m_argv; + +public: + ArgumentValuesHolder(int argc, std::vector<std::string> vals) + : m_argc(argc) + , m_vals(std::move(vals)) + , m_argv(new char *[argc]) + { + + int idx = 0; + for (auto& val: m_vals) { + m_argv[idx++] = &val.front(); + } + } + + ~ArgumentValuesHolder() { + delete[] m_argv; + } + + ArgumentValuesHolder(const ArgumentValuesHolder &other) = delete; + + ArgumentValuesHolder(ArgumentValuesHolder &&other) = delete; + + ArgumentValuesHolder &operator=(const ArgumentValuesHolder &other) = delete; + + ArgumentValuesHolder &operator=(ArgumentValuesHolder &&other) = delete; + + char** get_argv() { + return m_argv; + } +}; + +/** + * Creates modified copy of argument values in order to influence test framework behavior for different versions. + * @param argc Argument count. + * @param argv Argument values. + * @param version Compatibility version. + * @return An object which contains modified argument values. + */ +ArgumentValuesHolder override_xml_output_parameter(int argc, char **argv, const std::string &version) { + std::vector<std::string> vals; + vals.reserve(argc); + for (int i = 0; i < argc; ++i) { + std::string s = argv[i]; + + if (s.find("--gtest_output=xml:") == 0) { + if (auto pos = s.rfind(".xml"); pos == s.size() - 4) { + s.insert(pos, version); + } + } + + vals.push_back(s); + } + + return ArgumentValuesHolder{argc, std::move(vals)}; +} + +/** + * Replaces default xml printer with ours which adds version information to report. + * @param version Compatibility server version + */ +void replace_xml_print(const std::string &version) { + auto &test_event_listeners = ::testing::UnitTest::GetInstance()->listeners(); + + ::testing::TestEventListener *xml = test_event_listeners.default_xml_generator(); + + if (xml) { + test_event_listeners.Release(xml); + + test_event_listeners.Append(new detail::ignite_xml_unit_test_result_printer(xml, version)); + } +} + +int run_tests(int argc, char **argv, const std::string &version) { + // In case when RUN_ALL_TESTS() not started; + int res = 4; + try { + std::cout << "[**********] " << "Compatibility suite for version " << version << " started." << std::endl; + + auto holder = override_xml_output_parameter(argc, argv, version); + + ::testing::InitGoogleTest(&argc, holder.get_argv()); + + replace_xml_print(version); + + res = RUN_ALL_TESTS(); + + std::cout << "[**********] " << "Compatibility suite for version " << version << " finished" << std::endl; + } catch (const std::exception &err) { + std::cout << "[" << version << "] Uncaught error: " << err.what() << std::endl; + res = std::min(res, 2); + } catch (...) { + std::cout << "[" << version << "] Unknown uncaught error" << std::endl; + res = std::min(res, 3); + } + + return res; +} + +int main(int argc, char **argv) { + auto ver_override = detail::get_env("IGNITE_CPP_TESTS_COMPATIBILITY_VERSIONS_OVERRIDE"); + + const auto &versions = ver_override + ? [&ver_override]() { + std::cout << "Parsing compatibility version override: " << *ver_override << "\n"; + + std::vector<std::string> v; + size_t l = 0; + size_t r; + do { + r = ver_override->find(',', l); + v.push_back(ver_override->substr(l, r - l)); + l = r + 1; + } while (r != std::string::npos); + + return v; + }() + : DEFAULT_VERSIONS; + + int res = 0; + for (size_t i = 0; i < versions.size(); ++i) { + const std::string& version = versions[i]; + + ignite_runner runner{version}; + + set_process_abort_handler([&](int signal) { + std::cout << "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(90)); + } + int run_res; +#ifdef _WIN32 + run_res = run_tests(argc, argv, version); +#else + + // When debugging we most likely would choose one version + // For debug reasons last version will run in the same process + if (i == versions.size() - 1) { + run_res = run_tests(argc, argv, version); + } else { + // We fork because GTest initializes a lot of global variables which interfere with subsequent runs. + int pid = ::fork(); + + if (pid == -1) { + std::stringstream ss; + ss << "Can't fork proces for version " << version; Review Comment: Corrected spelling of 'proces' to 'process'. ```suggestion ss << "Can't fork process for version " << version; ``` ########## modules/platforms/cpp/tests/compatibility-tests/ignite_xml_unit_test_result_printer.cpp: ########## @@ -0,0 +1,127 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +#include "ignite_xml_unit_test_result_printer.h" + +namespace ignite::detail { + +ignite_xml_unit_test_result_printer::ignite_xml_unit_test_result_printer( + testing::TestEventListener *delegate, std::string version) + : m_delegate(delegate) + , m_version(std::move(version)){} + +void ignite_xml_unit_test_result_printer::OnTestProgramStart(const ::testing::UnitTest &unit_test) { + m_delegate->OnTestProgramStart(unit_test); +} + +void ignite_xml_unit_test_result_printer::OnTestIterationStart(const ::testing::UnitTest &unit_test, int iteration) { + m_delegate->OnTestIterationStart(unit_test, iteration); +} + +void ignite_xml_unit_test_result_printer::OnEnvironmentsSetUpStart(const ::testing::UnitTest &unit_test) { + m_delegate->OnEnvironmentsSetUpStart(unit_test); +} + +void ignite_xml_unit_test_result_printer::OnEnvironmentsSetUpEnd(const ::testing::UnitTest &unit_test) { + m_delegate->OnEnvironmentsSetUpEnd(unit_test); +} + +void ignite_xml_unit_test_result_printer::OnTestSuiteStart(const ::testing::TestSuite &test_suite) { + m_delegate->OnTestSuiteStart(test_suite); +} + +void ignite_xml_unit_test_result_printer::OnTestSuiteEnd(const ::testing::TestSuite &test_suite) { + m_delegate->OnTestSuiteEnd(test_suite); +} + +void ignite_xml_unit_test_result_printer::OnTestCaseStart(const ::testing::TestCase &test_case) { + m_delegate->OnTestCaseStart(test_case); +} + +void ignite_xml_unit_test_result_printer::OnTestCaseEnd(const ::testing::TestCase &test_case) { + m_delegate->OnTestCaseEnd(test_case); +} + +void ignite_xml_unit_test_result_printer::OnTestStart(const ::testing::TestInfo &test_info) { + m_delegate->OnTestStart(test_info); +} + +void ignite_xml_unit_test_result_printer::OnTestDisabled(const testing::TestInfo &test_info) { + m_delegate->OnTestDisabled(test_info); +} + +void ignite_xml_unit_test_result_printer::OnTestPartResult(const ::testing::TestPartResult &test_part_result) { + m_delegate->OnTestPartResult(test_part_result); +} + +void ignite_xml_unit_test_result_printer::OnTestEnd(const ::testing::TestInfo &test_info) { + m_delegate->OnTestEnd(test_info); +} + +void ignite_xml_unit_test_result_printer::OnEnvironmentsTearDownStart(const ::testing::UnitTest &unit_test) { + m_delegate->OnEnvironmentsTearDownStart(unit_test); +} + +void ignite_xml_unit_test_result_printer::OnEnvironmentsTearDownEnd(const ::testing::UnitTest &unit_test) { + m_delegate->OnEnvironmentsTearDownEnd(unit_test); +} + +void ignite_xml_unit_test_result_printer::OnTestIterationEnd(const ::testing::UnitTest &unit_test, int iteration) { + for (int i = 0; i < unit_test.total_test_case_count(); ++i) { + // We are extracting test suite info to add version info + const testing::TestSuite *ts = unit_test.GetTestSuite(i); + + // because underlying storage is std::string we able to override it content without changing length. + char *s = const_cast<char *>(ts->name()); + + std::string_view sw = s; + + std::string_view suffix = "_ign_version"; + + if (sw.rfind(suffix) != sw.size() - suffix.size()) { + std::stringstream ss; + ss << "Expected test suite name to have prefix '"<< suffix <<"' but got [name = "<< sw << "]"; Review Comment: The error message says 'prefix' but should say 'suffix' since the code is checking for a suffix at the end of the test suite name. ```suggestion ss << "Expected test suite name to have suffix '"<< suffix <<"' but got [name = "<< sw << "]"; ``` ########## modules/platforms/cpp/tests/compatibility-tests/main.cpp: ########## @@ -0,0 +1,236 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +#include "ignite_runner.h" +#include "ignite_xml_unit_test_result_printer.h" +#include "test_utils.h" + +#include <gtest/gtest.h> + +#include <chrono> +#include <csignal> +#include <cstring> + +namespace { + +/** Shutdown handler that cleans up resources. */ +std::function<void(int)> shutdown_handler; + +/** + * Receives OS signal and handles it. + * + * @param signum Signal value. + */ +void signal_handler(int signum) { + shutdown_handler(signum); + signal(signum, SIG_DFL); + raise(signum); +} + +} // namespace + +/** + * Sets process abortion (SIGABRT, SIGINT, SIGSEGV signals) handler. + * + * @param handler Abortion handler. + */ +void set_process_abort_handler(std::function<void(int)> handler) { + shutdown_handler = std::move(handler); + + // Install signal handlers to clean up resources on early exit. + signal(SIGABRT, signal_handler); + signal(SIGINT, signal_handler); + signal(SIGSEGV, signal_handler); +} + +using namespace ignite; + +const std::vector<std::string> DEFAULT_VERSIONS = {"9.1.0", "9.1.1", "9.1.2", "9.1.3", "9.1.4"}; + +/** + * Structure to store argument values for automatic memory management. + */ +struct ArgumentValuesHolder { +private: + int m_argc; + std::vector<std::string> m_vals; + char **m_argv; + +public: + ArgumentValuesHolder(int argc, std::vector<std::string> vals) + : m_argc(argc) + , m_vals(std::move(vals)) + , m_argv(new char *[argc]) + { + + int idx = 0; + for (auto& val: m_vals) { + m_argv[idx++] = &val.front(); + } + } + + ~ArgumentValuesHolder() { + delete[] m_argv; + } + + ArgumentValuesHolder(const ArgumentValuesHolder &other) = delete; + + ArgumentValuesHolder(ArgumentValuesHolder &&other) = delete; + + ArgumentValuesHolder &operator=(const ArgumentValuesHolder &other) = delete; + + ArgumentValuesHolder &operator=(ArgumentValuesHolder &&other) = delete; + + char** get_argv() { + return m_argv; + } +}; + +/** + * Creates modified copy of argument values in order to influence test framework behavior for different versions. + * @param argc Argument count. + * @param argv Argument values. + * @param version Compatibility version. + * @return An object which contains modified argument values. + */ +ArgumentValuesHolder override_xml_output_parameter(int argc, char **argv, const std::string &version) { + std::vector<std::string> vals; + vals.reserve(argc); + for (int i = 0; i < argc; ++i) { + std::string s = argv[i]; + + if (s.find("--gtest_output=xml:") == 0) { + if (auto pos = s.rfind(".xml"); pos == s.size() - 4) { + s.insert(pos, version); + } + } + + vals.push_back(s); + } + + return ArgumentValuesHolder{argc, std::move(vals)}; +} + +/** + * Replaces default xml printer with ours which adds version information to report. + * @param version Compatibility server version + */ +void replace_xml_print(const std::string &version) { + auto &test_event_listeners = ::testing::UnitTest::GetInstance()->listeners(); + + ::testing::TestEventListener *xml = test_event_listeners.default_xml_generator(); + + if (xml) { + test_event_listeners.Release(xml); + + test_event_listeners.Append(new detail::ignite_xml_unit_test_result_printer(xml, version)); + } +} + +int run_tests(int argc, char **argv, const std::string &version) { + // In case when RUN_ALL_TESTS() not started; + int res = 4; + try { + std::cout << "[**********] " << "Compatibility suite for version " << version << " started." << std::endl; + + auto holder = override_xml_output_parameter(argc, argv, version); + + ::testing::InitGoogleTest(&argc, holder.get_argv()); + + replace_xml_print(version); + + res = RUN_ALL_TESTS(); + + std::cout << "[**********] " << "Compatibility suite for version " << version << " finished" << std::endl; + } catch (const std::exception &err) { + std::cout << "[" << version << "] Uncaught error: " << err.what() << std::endl; + res = std::min(res, 2); + } catch (...) { + std::cout << "[" << version << "] Unknown uncaught error" << std::endl; + res = std::min(res, 3); + } + + return res; +} + +int main(int argc, char **argv) { + auto ver_override = detail::get_env("IGNITE_CPP_TESTS_COMPATIBILITY_VERSIONS_OVERRIDE"); + + const auto &versions = ver_override + ? [&ver_override]() { + std::cout << "Parsing compatibility version override: " << *ver_override << "\n"; + + std::vector<std::string> v; + size_t l = 0; + size_t r; + do { + r = ver_override->find(',', l); + v.push_back(ver_override->substr(l, r - l)); + l = r + 1; + } while (r != std::string::npos); + + return v; + }() + : DEFAULT_VERSIONS; + + int res = 0; + for (size_t i = 0; i < versions.size(); ++i) { + const std::string& version = versions[i]; + + ignite_runner runner{version}; + + set_process_abort_handler([&](int signal) { + std::cout << "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(90)); + } + int run_res; +#ifdef _WIN32 + run_res = run_tests(argc, argv, version); +#else + + // When debugging we most likely would choose one version + // For debug reasons last version will run in the same process + if (i == versions.size() - 1) { + run_res = run_tests(argc, argv, version); + } else { + // We fork because GTest initializes a lot of global variables which interfere with subsequent runs. + int pid = ::fork(); + + if (pid == -1) { + std::stringstream ss; + ss << "Can't fork proces for version " << version; + throw std::runtime_error(ss.str()); + } + + if (pid == 0) { + run_res = run_tests(argc, argv, version); + break; + } + + waitpid(pid, &run_res, 0); + } +#endif + res = std::min(res, run_res); Review Comment: The logic appears inverted: std::min will select the smaller value, but test results typically use 0 for success and non-zero for failure. This would cause a successful test (0) to override a failed test (non-zero), hiding failures. Should use std::max to preserve failure status. ```suggestion res = std::max(res, run_res); ``` -- 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]
