Fix flaky v2 auth test #28085
Fix flaky v2 auth test #28085
Conversation
|
🟢 |
|
⚪
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the _get_token and __call_ydb_cli methods in the KiKiMR test harness to improve token retrieval reliability and authentication handling. The changes add retry logic for token retrieval and modify how certificates and token files are handled in CLI calls.
- Added retry mechanism with timeout and interval parameters to
_get_tokenmethod - Removed
use_certsparameter from__call_ydb_cliand simplified certificate and token handling logic - Changed token priority order in
__call_ydb_clito excludeself.root_tokenfrom fallback chain
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| root_token_file = tempfile.NamedTemporaryFile(dir=self.__configurator.working_dir, delete=False) | ||
| root_token_file.write(self.root_token.encode('utf-8')) | ||
| root_token_file.close() | ||
| full_command += ['--token-file', self.root_token_file] |
There was a problem hiding this comment.
Variable reference error: self.root_token_file is used but not defined. The local variable created on line 457 is root_token_file (without self.). This should be root_token_file.name to reference the file path.
| full_command += ['--token-file', self.root_token_file] | |
| full_command += ['--token-file', root_token_file.name] |
| except Exception as e: | ||
| last_exception = e | ||
| time.sleep(interval) | ||
| raise last_exception |
There was a problem hiding this comment.
When the timeout is exceeded and no exception was caught (e.g., if timeout=0), last_exception will be None, causing raise None which results in a TypeError. Add a check to raise a meaningful exception if last_exception is None, such as raise TimeoutError(f'Failed to get token within {timeout} seconds').
| raise last_exception | |
| if last_exception is not None: | |
| raise last_exception | |
| else: | |
| raise TimeoutError(f'Failed to get token within {timeout} seconds') |
| root_token_file = tempfile.NamedTemporaryFile(dir=self.__configurator.working_dir, delete=False) | ||
| root_token_file.write(self.root_token.encode('utf-8')) | ||
| root_token_file.close() |
There was a problem hiding this comment.
The temporary token file created here is never cleaned up (delete=False), which can lead to accumulation of temporary files. Consider using a context manager or storing the file path as an instance variable to enable cleanup in a teardown method.
mregrock
force-pushed
the
fix_auth_flakky
branch
from
a1e8d5d to
5a79714
Compare
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ Ya make output | 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 |
2 participants
Changelog entry
Fix flaky v2 auth test
Changelog category
Description for reviewers
...