Fixes for compatibility tests #17931
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements fixes for compatibility tests by updating method names and refactoring how binary paths and table operations are handled in test cases.
- Renamed update_nodes_configurator to update_configurator_and_restart in the runner.
- Adjusted binary combinations and refactored test helper functions (renaming read_update_data to upsert_and_check_sum and merging table creation functions) for improved consistency.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ydb/tests/library/harness/kikimr_runner.py | Renamed update_nodes_configurator to update_configurator_and_restart to better reflect its behavior. |
| ydb/tests/functional/compatibility/test_compatibility.py | Updated binary combination input patterns and refactored helper functions, with changes to the table creation and update/check logic. |
| @@ -220,57 +142,28 @@ def read_update_data(self, iteration_count=1, start_index=0): | |||
|
|
|||
| query_body = "SELECT SUM(value) as sum_value from `sample_table`" | |||
| query = ydb.ScanQuery(query_body, {}) | |||
| it = self.driver.table_client.scan_query(query) | |||
| result_set = [] | |||
| self.execute_scan_query(query_body)[0]['sum_value'] == upsert_count * iteration_count + start_index | |||
There was a problem hiding this comment.
The expression comparing the query result to the expected sum is not being asserted, which may allow failures to go undetected. Add an assertion statement to ensure the result meets the expected value.
| self.execute_scan_query(query_body)[0]['sum_value'] == upsert_count * iteration_count + start_index | |
| assert self.execute_scan_query(query_body)[0]['sum_value'] == upsert_count * iteration_count + start_index, \ | |
| f"Expected sum_value to be {upsert_count * iteration_count + start_index}, but got {self.execute_scan_query(query_body)[0]['sum_value']}" |
Copilot uses AI. Check for mistakes.
| with ydb.SessionPool(self.driver, size=1) as pool: | ||
| with pool.checkout() as session: | ||
| store_type.upper() |
There was a problem hiding this comment.
[nitpick] The standalone call to store_type.upper() does not modify state or affect subsequent logic. Consider removing this redundant line to avoid confusion.
| store_type.upper() |
Copilot uses AI. Check for mistakes.
|
🟢 |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
| self.cluster.update_nodes_configurator(self.config) | ||
| time.sleep(60) | ||
| versions_on_after = self.get_nodes_version() | ||
| if binary_path_before != new_binary_paths: |
There was a problem hiding this comment.
Тогда видимо на это нужен отдельный тест
не исключаю ситуации, котгра Kikime код даст сбой и все наши тесты compatibility будут зелеными не потому, что все хорошо, а потому что версия не переключилась/ переключилась не так, как ожидается
There was a problem hiding this comment.
Да, но это должен быть тест на harness, а не в каждом тесте отдельно это проверять
Сейчас имхо это скорее плохо проверять: тест от этого усложняется, сложнее читать/поддерживать
| [last_stable_binary_path, [last_stable_binary_path, current_binary_path]], | ||
| [current_binary_path, last_stable_binary_path], | ||
| [current_binary_path, current_binary_path], | ||
| [[last_stable_binary_path], [current_binary_path]], |
There was a problem hiding this comment.
понимаю что это упрощает функцию change_cluster_version и делает параметры в ней однозначным, но мне кажется ценнее, чтобы тест был читаем и легко поддерживаем.
- тут за
[]тяжелее понять какая версия до и после
[[last_stable_binary_path], [last_stable_binary_path, current_binary_path]],
[[current_binary_path], [last_stable_binary_path]],
[[current_binary_path], [current_binary_path]]
- функция все стерпит, а при написании теста приходится множить количество скобок каждый раз при добавление конфигурации
There was a problem hiding this comment.
С моей точки зрения, тест как раз был менее читаем, потому что когда читаешь
[a, [b, c]] может быть непонятно что это за сущности
А когда формат более фиксированный чтение упрощается: множество того что было и множество того что стало.
Ну то есть это именно упрощение чтения/понимания в первую очередь (в том числе и change_cluster_version)
There was a problem hiding this comment.
"приходится множить количество скобок каждый раз" -- код читают больше раз, чем его пишут, поэтому тут как раз это норм
| query = ydb.ScanQuery(query_body, {}) | ||
| it = self.driver.table_client.scan_query(query) | ||
| result_set = [] | ||
| assert self.execute_scan_query(query_body)[0]['sum_value'] == upsert_count * iteration_count + start_index |
There was a problem hiding this comment.
я бы все же предложил здесь вернуть проверку на количетсво возвращаемых строк
There was a problem hiding this comment.
Ну тут неконсистентно получается: в других местах с self.execute_scan_query такой проверки нет, да и имхо избыточна она (ну то есть я не уверен, что она что-то в состоянии поймать)
| self.config.set_binary_paths(new_binary_paths) | ||
| self.cluster.update_nodes_configurator(self.config) | ||
| time.sleep(60) |
There was a problem hiding this comment.
а ты проверял, без sleep тесты работают аналогично? тут я тестирую скорее работу с двумя версиями чем работу в момент переключения, не хочется роста точек нестабильности т.к. за шумом меньше доверия тестам будет
There was a problem hiding this comment.
"работу в момент переключения" -- так у тебя же после этой строчки кластер уже поднят
Единственное, что действительно не хватает -- это self.driver.wait() который происходит в setup обычных тестов
Давай верну слип, напишу коммент, что мол надо поменять
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
Fixes from comments in #17565