feat(integration-tests): Clean up logging and error reporting in integration tests.#1802
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe changes implement a comprehensive logging infrastructure for integration tests, including per-run timestamped log directories, structured subprocess output capture, colorized test reporting, and refactored subprocess handling utilities. Configuration updates enable detailed test execution logging while new utilities centralize subprocess execution and output logging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
with the understanding that this is a draft, i just took a quick look
| .task/ | ||
| build/ | ||
| dist/ | ||
| __* No newline at end of file |
There was a problem hiding this comment.
what are we trying to exclude here?
There was a problem hiding this comment.
The __pytest_logs directory that stores the log file for each test run.
That being said, right now the __pytest_logs directory gets created/stored under the clp/integration-tests directory. Do you think it might be better to have it under the build/integration-tests directory instead (used for other integration test stuff as well)? Then we wouldn't have to modify this file.
There was a problem hiding this comment.
yup i agree we can move to build/
in general we want to be more explicit about what to ignore, so if we keep the folder in the current location, it would be better to just write __pytest_logs. That said, it really depends on which part of the paths we want to be fuzzy about. in general wildcards like path/*/subpath and *.log are fine in my opinion
| log_cli_date_format = %Y-%m-%d %H:%M:%S,%f | ||
| log_cli_format = %(name)s %(asctime)s [%(levelname)s] %(message)s | ||
| log_cli_level = INFO | ||
| log_cli_format = %(levelname)-8s %(asctime)-25s %(message)s |
There was a problem hiding this comment.
any reason to change this? i thought we were trying to match references like
There was a problem hiding this comment.
My only reason was aesthetics, cause this is the format of the logs that will be reported to the user via the command line. If there is some reason we want to match references like you describe above, I can change it; I just wasn't aware we were doing that
|
|
||
| log_file_mode = a | ||
| log_file_level = DEBUG | ||
| log_file_format = %(levelname)-8s %(asctime)-30s [%(name)s:%(filename)s:%(lineno)d]: %(message)s |
There was a problem hiding this comment.
any reason to use a different format string between the cli and the log file?
There was a problem hiding this comment.
Just to provide more information to the dev; I don't want to crowd the cli output, especially for a successful (no failure) test run. I think it's safe to assume that the main reason a dev would be looking at the test log would be if something fails, and in the event of failure, they'll need more information.
| log_cli_format = %(levelname)-8s %(asctime)-25s %(message)s | ||
| log_cli_date_format = %Y-%m-%d %H:%M:%S | ||
|
|
||
| log_file_mode = a |
There was a problem hiding this comment.
instead of appending to the same file, can isolate the logs from different test runs instead? e.g., use set_log_path with the default log_file_mode = w. see the test code at pytest-dev/pytest#4752 for example
There was a problem hiding this comment.
If this isn't here, the test output file will contain all of the pytest logging messages followed by all of the output from the various subprocesses called during the test run, like this:
INFO 2025-01-06 event1
INFO 2025-01-06 event2
INFO 2025-01-06 event3
<output following event 1>
<output following event 2>
<output following event 3>
It seems better to me to have everything written to the log file in order, so that it looks like this:
INFO 2025-01-06 event1
<output following event 1>
INFO 2025-01-06 event2
<output following event 2>
INFO 2025-01-06 event3
<output following event 3>
The logs from different test runs are already isolated by the code in conftest.py.
|
|
||
| def run_and_assert(cmd: list[str], **kwargs: Any) -> subprocess.CompletedProcess[Any]: | ||
| def run_and_assert( | ||
| request: pytest.FixtureRequest, cmd: list[str], **kwargs: Any |
There was a problem hiding this comment.
update below docstring for the param
There was a problem hiding this comment.
do you mean the request? if so, right on, thank you
|
|
||
|
|
||
| def stop_clp_package(package_config: PackageConfig) -> None: | ||
| def stop_clp_package(request: pytest.FixtureRequest, package_config: PackageConfig) -> None: |
There was a problem hiding this comment.
right on, thanks
|
|
||
| try: | ||
| run_and_assert(request, stop_cmd, timeout=DEFAULT_CMD_TIMEOUT_SECONDS) | ||
| except SubprocessError: |
There was a problem hiding this comment.
in what case would this be thrown here? i thought we would have caught this in run_and_assert? like also if there are other SubprocessError than CalledProcessError that might be thrown but uncaught in run_and_assert, why don't we modify run_and_assert to handle such in the first place?
There was a problem hiding this comment.
Yeah that's a good point. See my message that I sent you offline.
There was a problem hiding this comment.
|
|
||
| try: | ||
| run_and_assert(request, start_cmd, timeout=DEFAULT_CMD_TIMEOUT_SECONDS) | ||
| except SubprocessError: |
There was a problem hiding this comment.
ditto -
in what case would this be thrown here? i thought we would have caught this in
run_and_assert? like also if there are otherSubprocessErrorthanCalledProcessErrorthat might be thrown but uncaught inrun_and_assert, why don't we modifyrun_and_assertto handle such in the first place?
There was a problem hiding this comment.
ditto my other comment -
|
|
||
|
|
||
| def start_clp_package(package_config: PackageConfig) -> None: | ||
| def start_clp_package(request: pytest.FixtureRequest, package_config: PackageConfig) -> None: |
There was a problem hiding this comment.
right on, thanks
quinntaylormitchell
requested review from
junhaoliao
and removed request for
junhaoliao
quinntaylormitchell
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@integration-tests/tests/conftest.py`:
- Around line 33-41: Change the parser.addini for "log_file_path" to use
type="string" (not "paths") and keep the default as the generated Path string;
update code that calls Path(config.getini("log_file_path")) to handle a string
return (no list) — look for uses of config.getini("log_file_path") in this file
and where the path is constructed. Also ensure the parent directory
"__pytest_logs" is created before any file write by adding a directory-creation
step (mkdir(parents=True, exist_ok=True)) at the point where the log file path
is first resolved/used (e.g., just after building log_file_path in this module
and before any write operations in asserting_utils.py).
- Around line 44-51: The current pytest_itemcollected hook overwrites
item._nodeid using item.name which loses module/class context; update the
assignment in pytest_itemcollected to use item.nodeid instead of item.name so
the prettified _nodeid preserves the full, globally unique identifier (i.e., set
item._nodeid = f"{BOLD}{BLUE}{item.nodeid}{RESET}" inside the
pytest_itemcollected function).
In `@integration-tests/tests/utils/logging_utils.py`:
- Line 5: Remove the unused GREEN constant from
integration-tests/tests/utils/logging_utils.py to eliminate dead code: locate
the symbol GREEN and delete its definition (the "\033[32m" assignment) or, if
it's intended for future use, add a clear explanatory comment above the GREEN
declaration indicating why it's kept; ensure no imports or references to GREEN
exist elsewhere before removal.
In `@integration-tests/tests/utils/package_utils.py`:
- Around line 38-44: The except block around run_and_assert should catch
OS-level launch failures and include the exception text: change the handler from
except SubprocessError: to except (SubprocessError, OSError) as e and include
str(e) in the error log and pytest.fail message so missing/non-executable
scripts are surfaced; update the logger.error call that uses
construct_log_err_msg(err_msg) to include the exception string (e.g.,
f"{err_msg} Error: {e}") and similarly append the exception text to the
pytest.fail call; relevant symbols: run_and_assert, SubprocessError,
package_test_config.mode_config.mode_name, construct_log_err_msg,
DEFAULT_CMD_TIMEOUT_SECONDS.
junhaoliao
left a comment
junhaoliao
left a comment
There was a problem hiding this comment.
no major issue found except the log file writing concurrency issue. i originally proposed https://github.com/y-scope/clp/pull/1802/files#r2660585892 to solve the issue, but now i agree with the rationale for keeping all logs in the same file. However, the concurrency problem is still valid, please see if this proposal works - "would it be better to use just Python loggers to log the output of the sub processes"
| .task/ | ||
| build/ | ||
| dist/ | ||
| __* No newline at end of file |
There was a problem hiding this comment.
yup i agree we can move to build/
in general we want to be more explicit about what to ignore, so if we keep the folder in the current location, it would be better to just write __pytest_logs. That said, it really depends on which part of the paths we want to be fuzzy about. in general wildcards like path/*/subpath and *.log are fine in my opinion
| .task/ | ||
| build/ | ||
| dist/ | ||
| __* |
There was a problem hiding this comment.
(some response to a previous discussion somehow refuses to be shown in the "Files changed" tab)
duplicating the comment here for visibility -
yup i agree we can move to build/
in general we want to be more explicit about what to ignore, so if we keep the folder in the current location, it would be better to just write __pytest_logs. That said, it really depends on which part of the paths we want to be fuzzy about. in general wildcards like path/*/subpath and *.log are fine in my opinion
There was a problem hiding this comment.
moved to build/
| "log_file_path", | ||
| help="Path to the log file for this test.", | ||
| type="paths", | ||
| default=log_file_path, |
There was a problem hiding this comment.
seems like with type="paths", the parser returns a list of Path objects when we actually specify any value in the config file?
shall we use [log_file_path] or change to type="string" which might make more sense?
There was a problem hiding this comment.
I'll just store it as a string
| pytest.fail(f"Command timed out: {' '.join(cmd)}: {e}") | ||
| return proc | ||
| log_file_path = Path(request.config.getini("log_file_path")) | ||
| with log_file_path.open("ab") as log_file: |
There was a problem hiding this comment.
the pytest logging plugin writes to the same file handle, right? any concurrency concerns?
I understand the outputs are usually flushed only on newlines, but this feels a bit fragile.
would it be better to use just Python loggers to log the output of the sub processes once any content is available?
something like opening a (combined) stream then iterating line-by-line should work?
proc = subprocess.Popen(
cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, **kwargs
)
for line in proc.stdout:
logger.debug("%s", line.decode(errors="replace").rstrip())
proc.wait()
There was a problem hiding this comment.
btw, it might be worth using a prefix in the logs otherwise we easily get mixed contents from different commands if we ever call this multiple times concurrently.
e.g.
logger.debug("[%s] %s", cmd, line.decode...)
There was a problem hiding this comment.
I don't really have any concurrency concerns here, because pytest only runs sequentially (the only way to run it in parallel is to use plugins that we don't use). With that in mind, it seems to me like there is no way that the logging file would be accessed by multiple processes at the same time, and by the same token, there's no way that this would get called multiple times concurrently
|
|
||
| def start_clp_package(package_test_config: PackageTestConfig) -> None: | ||
| def start_clp_package( | ||
| request: pytest.FixtureRequest, package_test_config: PackageTestConfig |
There was a problem hiding this comment.
right on
|
|
||
| def stop_clp_package(package_test_config: PackageTestConfig) -> None: | ||
| def stop_clp_package( | ||
| request: pytest.FixtureRequest, package_test_config: PackageTestConfig |
There was a problem hiding this comment.
right on
| RESET = "\033[0m" | ||
|
|
||
|
|
||
| def construct_log_err_msg(err_msg: str) -> str: |
There was a problem hiding this comment.
missing param / return dosctring
There was a problem hiding this comment.
thank u
|
|
||
| :param item: | ||
| """ | ||
| item._nodeid = f"{BOLD}{BLUE}{item.name}{RESET}" # noqa: SLF001 |
There was a problem hiding this comment.
just a heads up -
The ANSI constants are terminal (print("...")) formatting concerns, not logging (logger.info("...")) concerns.
We usually don't want to mix them together, though here the complexity is fine to be tolerated as i'm fine keeping it if you think it helps visualization. Normally, if we want facier colorful outputs, we should look into creating a custom LoggerAdapter like explained by https://docs.python.org/3/howto/logging-cookbook.html
(anyways, don't bother until it becomes more complex like we don't only add colorful prefixes but also colorful postfixes, lol
There was a problem hiding this comment.
thanks, I'll keep that in mind
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
integration-tests/tests/utils/package_utils.py (2)
18-45: 🧹 Nitpick | 🔵 TrivialInclude exception details in error message for easier debugging.
The exception handler catches
SubprocessErrorandOSErrorbut discards the exception details. Including the exception text would help identify the root cause (e.g., missing script, permission denied, non-zero exit code) without requiring users to dig through log files.Proposed fix
try: run_and_log_to_file(request, start_cmd, timeout=DEFAULT_CMD_TIMEOUT_SECONDS) - except (SubprocessError, OSError): + except (SubprocessError, OSError) as err: mode_name = package_test_config.mode_config.mode_name - err_msg = f"The '{mode_name}' package failed to start." + err_msg = f"The '{mode_name}' package failed to start: {err}" logger.error(construct_log_err_msg(err_msg)) pytest.fail(err_msg)
48-75: 🧹 Nitpick | 🔵 TrivialSame suggestion: include exception details in stop error message.
Apply the same pattern to the stop function for consistency.
Proposed fix
try: run_and_log_to_file(request, stop_cmd, timeout=DEFAULT_CMD_TIMEOUT_SECONDS) - except (SubprocessError, OSError): + except (SubprocessError, OSError) as err: mode_name = package_test_config.mode_config.mode_name - err_msg = f"The '{mode_name}' package failed to stop." + err_msg = f"The '{mode_name}' package failed to stop: {err}" logger.error(construct_log_err_msg(err_msg)) pytest.fail(err_msg)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
integration-tests/tests/package_tests/clp_text/test_clp_text.py (1)
19-19:⚠️ Potential issue | 🔴 CriticalRestore the
loggingimport or drop the module logger.
logger = logging.getLogger(__name__)runs at import time, so this module currently raisesNameErrorbefore pytest can collect the tests.Possible fix
+import logging import pytest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/tests/package_tests/clp_text/test_clp_text.py` at line 19, The module-level statement logger = logging.getLogger(__name__) runs at import time but logging is not imported, causing a NameError during pytest collection; either add an import logging at the top of the module or remove/replace the module-level logger usage (the line containing logger = logging.getLogger(__name__)) with local logging calls or pass-through to pytest's caplog. Update the file so that logging is defined before logger is created and ensure any references to logger use the newly imported logging or are removed.integration-tests/tests/utils/package_utils.py (1)
12-39:⚠️ Potential issue | 🔴 CriticalThis startup helper introduces several undefined names.
pytest,run_and_log_to_file,SubprocessError,logger, andconstruct_log_err_msgare all referenced here without matching imports in this module. That leaves the new startup path broken before its error-reporting logic can run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/tests/utils/package_utils.py` around lines 12 - 39, The start_clp_package function references several names that aren't imported; add imports for pytest, SubprocessError, and the test logging/helpers used by run_and_log_to_file, construct_log_err_msg, and logger so the function can run and report errors: e.g. add "import pytest", "from subprocess import SubprocessError", and import run_and_log_to_file, construct_log_err_msg, and logger from whatever test utils/logging module provides them so start_clp_package can call run_and_log_to_file and log failures correctly.
♻️ Duplicate comments (1)
integration-tests/tests/utils/package_utils.py (1)
42-62:⚠️ Potential issue | 🔴 Critical
stop_clp_packageis still on the old execution path.
requestis unused here,stop-clp.shoutput still bypasses the per-test log file, and this function still depends onrun_and_assert. Migrate it to the same helper as startup so the new logging/reporting behaviour is applied consistently.Based on learnings, `run_and_assert` should only be used for CLP package functionality commands; helper commands should use the non-asserting subprocess helper instead.Possible fix
- run_and_assert(stop_cmd, timeout=DEFAULT_CMD_TIMEOUT_SECONDS) + run_and_log_to_file(request, stop_cmd, timeout=DEFAULT_CMD_TIMEOUT_SECONDS)
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration-tests/tests/utils/asserting_utils.py`:
- Around line 76-88: The failure paths call construct_log_err_msg but it is not
imported, causing a NameError; add an import for construct_log_err_msg at the
top of the module (alongside other imports) so the calls in the component
validation block (where err_msg is built and
logger.error(construct_log_err_msg(err_msg)) / pytest.fail(err_msg) are executed
correctly; ensure the import matches the module that defines
construct_log_err_msg and remove any duplicate or unused imports.
- Around line 21-34: The new helper run_and_log_to_file replaced run_and_assert
prematurely—restore a run_and_assert compatibility helper that preserves the
original behavior (exit-on-failure/assert semantics and return/exception
contract) so existing callers like verify_package_compression and functions in
tests.utils.package_utils continue to work; implement run_and_assert as either
the original implementation or a thin wrapper that calls run_and_log_to_file but
mimics the prior return/exception semantics and signature so no caller changes
are required until those tests are updated.
---
Outside diff comments:
In `@integration-tests/tests/package_tests/clp_text/test_clp_text.py`:
- Line 19: The module-level statement logger = logging.getLogger(__name__) runs
at import time but logging is not imported, causing a NameError during pytest
collection; either add an import logging at the top of the module or
remove/replace the module-level logger usage (the line containing logger =
logging.getLogger(__name__)) with local logging calls or pass-through to
pytest's caplog. Update the file so that logging is defined before logger is
created and ensure any references to logger use the newly imported logging or
are removed.
In `@integration-tests/tests/utils/package_utils.py`:
- Around line 12-39: The start_clp_package function references several names
that aren't imported; add imports for pytest, SubprocessError, and the test
logging/helpers used by run_and_log_to_file, construct_log_err_msg, and logger
so the function can run and report errors: e.g. add "import pytest", "from
subprocess import SubprocessError", and import run_and_log_to_file,
construct_log_err_msg, and logger from whatever test utils/logging module
provides them so start_clp_package can call run_and_log_to_file and log failures
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 789bab7a-4922-41e9-a0b8-51e9ef0895da
📒 Files selected for processing (6)
integration-tests/.pytest.iniintegration-tests/tests/package_tests/clp_json/test_clp_json.pyintegration-tests/tests/package_tests/clp_text/test_clp_text.pyintegration-tests/tests/utils/asserting_utils.pyintegration-tests/tests/utils/config.pyintegration-tests/tests/utils/package_utils.py
| def run_and_log_to_file(request: pytest.FixtureRequest, cmd: list[str], **kwargs: Any) -> None: | ||
| """ | ||
| Runs a command with subprocess and asserts that it succeeds with pytest. | ||
| Runs a command with subprocess. | ||
|
|
||
| :param request: Pytest fixture request. | ||
| :param cmd: Command and arguments to execute. | ||
| :param kwargs: Additional keyword arguments passed through to the subprocess. | ||
| :return: The completed process object, for inspection or further handling. | ||
| :raise: pytest.fail if the command exits with a non-zero return code. | ||
| :raise: Propagates `subprocess.run`'s errors. | ||
| """ | ||
| logger.info("Running command: %s", shlex.join(cmd)) | ||
|
|
||
| try: | ||
| proc = subprocess.run(cmd, check=True, **kwargs) | ||
| except subprocess.CalledProcessError as e: | ||
| pytest.fail(f"Command failed: {' '.join(cmd)}: {e}") | ||
| except subprocess.TimeoutExpired as e: | ||
| pytest.fail(f"Command timed out: {' '.join(cmd)}: {e}") | ||
| return proc | ||
| log_file_path = Path(request.config.getini("log_file_path")) | ||
| with log_file_path.open("ab") as log_file: | ||
| log_debug_msg = f"Now running command: {cmd}" | ||
| logger.debug(log_debug_msg) | ||
| subprocess.run(cmd, stdout=log_file, stderr=log_file, check=True, **kwargs) |
There was a problem hiding this comment.
Don't drop run_and_assert until the callers move.
This replacement is incomplete: verify_package_compression in this module and tests.utils.package_utils still reference run_and_assert. In the current state, those compression paths break before the new log-file flow can help.
Based on learnings, run_and_assert is still the expected helper for CLP package functionality commands such as compression and decompression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration-tests/tests/utils/asserting_utils.py` around lines 21 - 34, The
new helper run_and_log_to_file replaced run_and_assert prematurely—restore a
run_and_assert compatibility helper that preserves the original behavior
(exit-on-failure/assert semantics and return/exception contract) so existing
callers like verify_package_compression and functions in
tests.utils.package_utils continue to work; implement run_and_assert as either
the original implementation or a thin wrapper that calls run_and_log_to_file but
mimics the prior return/exception semantics and signature so no caller changes
are required until those tests are updated.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
integration-tests/tests/fixtures/package_test_config.py (1)
31-45: 🧹 Nitpick | 🔵 TrivialConsider lazy-evaluation formatting for log statements.
Similar to other files, the log statements use f-strings. Per project learnings, consider using printf-style formatting.
Suggested fix
- log_msg = f"Assigning ports to the '{mode_config.mode_name}' package." - logger.info(log_msg) + logger.info("Assigning ports to the '%s' package.", mode_config.mode_name) ... - log_msg = ( - f"Constructing the PackageTestConfig object for the '{mode_config.mode_name}' package." - ) - logger.info(log_msg) + logger.info( + "Constructing the PackageTestConfig object for the '%s' package.", + mode_config.mode_name, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/tests/fixtures/package_test_config.py` around lines 31 - 45, The logger.info calls in this block use f-strings (e.g., building log_msg with f"Assigning ports to the '{mode_config.mode_name}' package." and the subsequent construction message); change these to deferred/printf-style logging by passing the format string and arguments to logger.info (e.g., "Assigning ports to the '%s' package.", mode_config.mode_name) and similarly for the PackageTestConfig message, removing the intermediate f-string construction so the logger performs formatting lazily.integration-tests/tests/test_identity_transformation.py (1)
142-152: 🧹 Nitpick | 🔵 TrivialMissing docstring for helper function.
Per a past review comment,
_clp_s_compress_and_decompressshould have a docstring. The reviewer mentioned this would be addressed once the overarching approach was approved.Suggested docstring
def _clp_s_compress_and_decompress( clp_core_path_config: ClpCorePathConfig, test_paths: CompressionTestPathConfig, ) -> None: + """ + Compresses and decompresses using clp-s binary. + + :param clp_core_path_config: Configuration containing paths to CLP core binaries. + :param test_paths: Configuration containing test input/output paths. + """ test_paths.clear_test_outputs()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/tests/test_identity_transformation.py` around lines 142 - 152, The helper function _clp_s_compress_and_decompress is missing a docstring; add a concise triple-quoted docstring immediately after the def describing the function's purpose (compress source logs and then decompress them for tests), its parameters (clp_core_path_config: ClpCorePathConfig, test_paths: CompressionTestPathConfig), the side effects (clears test outputs, invokes run_and_log_subprocess to run the clp_s binary), and that it returns None; keep it brief and follow the module's docstring style for internal helper functions.integration-tests/tests/utils/asserting_utils.py (1)
102-112: 🧹 Nitpick | 🔵 TrivialAdd
:raise:documentation forrun_and_log_subprocesserrors.The docstring doesn't document that this function can propagate errors from
run_and_log_subprocess. For consistency with other functions in this codebase (e.g.,_validate_running_mode_correctdocuments its raises), consider adding this.📝 Proposed fix
def verify_package_compression( path_to_original_dataset: Path, package_test_config: PackageTestConfig, ) -> None: """ Verify that compression has been executed correctly by decompressing the contents of `clp-package/var/data/archives` and comparing the decompressed logs to the originals stored at `path_to_original_dataset`. :param path_to_original_dataset: :param package_test_config: + :raise: Propagates `run_and_log_subprocess`'s errors. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/tests/utils/asserting_utils.py` around lines 102 - 112, The docstring for this verification function is missing a :raise: entry documenting that it can propagate exceptions from run_and_log_subprocess; update the function's docstring (the one containing the current verification text and logger.info call) to include a :raise: clause stating that errors raised by run_and_log_subprocess (e.g., subprocess.CalledProcessError or other subprocess-related exceptions) may be propagated, mirroring the style used in _validate_running_mode_correct.integration-tests/tests/utils/package_utils.py (1)
56-61: 🧹 Nitpick | 🔵 TrivialAdd
:raise:documentation for consistency.Unlike
start_clp_packageandstop_clp_package, this function's docstring doesn't document that it propagates errors fromrun_and_log_subprocess.📝 Proposed fix
def run_package_compression_script( compression_job: PackageCompressionJob, package_test_config: PackageTestConfig, ) -> None: """ Constructs and runs a compression command on the CLP package. :param compression_job: :param package_test_config: + :raise: Propagates `run_and_log_subprocess`'s errors. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/tests/utils/package_utils.py` around lines 56 - 61, The docstring for the function that "Constructs and runs a compression command on the CLP package" should include a :raises: section, matching the style used in start_clp_package and stop_clp_package; document that this function propagates exceptions raised by run_and_log_subprocess (e.g., subprocess.CalledProcessError or generic Exception as appropriate), add a brief :raises: line to the function's docstring stating which exception(s) are raised and under what condition, and ensure the wording matches existing docstring conventions used for start_clp_package/stop_clp_package.
♻️ Duplicate comments (1)
integration-tests/tests/utils/logging_utils.py (1)
17-17: 🧹 Nitpick | 🔵 TrivialIncomplete docstring.
The docstring is a placeholder. Please add proper documentation including parameter descriptions and return value (or lack thereof).
Suggested docstring
- """Docstring.""" + """ + Logs subprocess output to a timestamped file in the test log directory. + + :param subprocess: The completed subprocess result containing stdout and stderr. + :param cmd: The command list that was executed. + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration-tests/tests/utils/logging_utils.py` at line 17, Replace the placeholder triple-quoted docstring in the logging_utils module with a proper module-level docstring that describes the module purpose and behavior, and add, for each public function in this file (e.g., configure_logger, get_test_logger, or any other exposed helpers), a short docstring explaining parameters (name and type) and return value (or None) and any raised exceptions; ensure the docstring follows the project's docstring style and documents side effects and usage examples if applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration-tests/tests/conftest.py`:
- Around line 76-83: The current pytest_itemcollected hook overwrites pytest's
internal item._nodeid with only item.name, losing path/context; change the
assignment to use item.nodeid (e.g., set item._nodeid to the formatted
item.nodeid) so the full test identity is preserved while keeping the same
BOLD/BLUE/RESET formatting; keep using item.name elsewhere for display-only
purposes but do not mutate _nodeid with item.name.
In `@integration-tests/tests/utils/logging_utils.py`:
- Around line 13-16: The parameter name subprocess in the function
log_subprocess_output_to_file shadows the imported subprocess module; rename
that parameter (e.g., to completed_process or proc) and update all references
inside the function accordingly so the module name remains available for future
use; ensure the function signature def
log_subprocess_output_to_file(completed_process:
subprocess.CompletedProcess[str], cmd: list[str]) -> None and any internal
uses/annotations are updated to the new name (and update any callers in this
file that pass/expect the old name).
In `@integration-tests/tests/utils/package_utils.py`:
- Line 9: DEFAULT_CMD_TIMEOUT_SECONDS in package_utils.py is dead code; either
remove the constant or make package_utils.py use it when invoking
run_and_log_subprocess (so the value isn't duplicated). Locate
DEFAULT_CMD_TIMEOUT_SECONDS and either delete its declaration or import/forward
it into calls to run_and_log_subprocess (or wire it into any helper that calls
subprocess_utils.run_and_log_subprocess) so the constant is referenced rather
than unused.
In `@integration-tests/tests/utils/subprocess_utils.py`:
- Around line 15-16: The docstring for run_and_log_subprocess is a placeholder;
replace it with a full docstring describing the function's purpose, parameters,
return value, and behavior (including that cmd is a list[str], it executes a
subprocess, logs stdout/stderr, and returns subprocess.CompletedProcess[str])
and mention any exceptions or side effects (e.g., logging, timeout behavior) so
callers understand usage; update the docstring directly above the
run_and_log_subprocess definition.
- Line 1: Replace the placeholder module docstring at the top of
subprocess_utils.py with a clear descriptive docstring that summarizes the
module's purpose (what utilities it provides for spawning/managing subprocesses
in tests), describes key behaviors or responsibilities, and notes any important
usage or side-effects; update the module-level string (the first triple-quoted
string) to include this information so readers and test authors immediately
understand the intent of subprocess_utils.py.
---
Outside diff comments:
In `@integration-tests/tests/fixtures/package_test_config.py`:
- Around line 31-45: The logger.info calls in this block use f-strings (e.g.,
building log_msg with f"Assigning ports to the '{mode_config.mode_name}'
package." and the subsequent construction message); change these to
deferred/printf-style logging by passing the format string and arguments to
logger.info (e.g., "Assigning ports to the '%s' package.",
mode_config.mode_name) and similarly for the PackageTestConfig message, removing
the intermediate f-string construction so the logger performs formatting lazily.
In `@integration-tests/tests/test_identity_transformation.py`:
- Around line 142-152: The helper function _clp_s_compress_and_decompress is
missing a docstring; add a concise triple-quoted docstring immediately after the
def describing the function's purpose (compress source logs and then decompress
them for tests), its parameters (clp_core_path_config: ClpCorePathConfig,
test_paths: CompressionTestPathConfig), the side effects (clears test outputs,
invokes run_and_log_subprocess to run the clp_s binary), and that it returns
None; keep it brief and follow the module's docstring style for internal helper
functions.
In `@integration-tests/tests/utils/asserting_utils.py`:
- Around line 102-112: The docstring for this verification function is missing a
:raise: entry documenting that it can propagate exceptions from
run_and_log_subprocess; update the function's docstring (the one containing the
current verification text and logger.info call) to include a :raise: clause
stating that errors raised by run_and_log_subprocess (e.g.,
subprocess.CalledProcessError or other subprocess-related exceptions) may be
propagated, mirroring the style used in _validate_running_mode_correct.
In `@integration-tests/tests/utils/package_utils.py`:
- Around line 56-61: The docstring for the function that "Constructs and runs a
compression command on the CLP package" should include a :raises: section,
matching the style used in start_clp_package and stop_clp_package; document that
this function propagates exceptions raised by run_and_log_subprocess (e.g.,
subprocess.CalledProcessError or generic Exception as appropriate), add a brief
:raises: line to the function's docstring stating which exception(s) are raised
and under what condition, and ensure the wording matches existing docstring
conventions used for start_clp_package/stop_clp_package.
---
Duplicate comments:
In `@integration-tests/tests/utils/logging_utils.py`:
- Line 17: Replace the placeholder triple-quoted docstring in the logging_utils
module with a proper module-level docstring that describes the module purpose
and behavior, and add, for each public function in this file (e.g.,
configure_logger, get_test_logger, or any other exposed helpers), a short
docstring explaining parameters (name and type) and return value (or None) and
any raised exceptions; ensure the docstring follows the project's docstring
style and documents side effects and usage examples if applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9835f2f1-b48d-419f-9f57-456f276a3dc8
📒 Files selected for processing (11)
integration-tests/.pytest.iniintegration-tests/tests/conftest.pyintegration-tests/tests/fixtures/package_test_config.pyintegration-tests/tests/package_tests/clp_json/test_clp_json.pyintegration-tests/tests/package_tests/clp_text/test_clp_text.pyintegration-tests/tests/test_identity_transformation.pyintegration-tests/tests/utils/asserting_utils.pyintegration-tests/tests/utils/config.pyintegration-tests/tests/utils/logging_utils.pyintegration-tests/tests/utils/package_utils.pyintegration-tests/tests/utils/subprocess_utils.py
💤 Files with no reviewable changes (1)
- integration-tests/tests/utils/config.py
| from tests.utils.subprocess_utils import run_and_log_subprocess | ||
|
|
||
|
|
||
| def start_clp_package(package_test_config: PackageTestConfig) -> None: |
There was a problem hiding this comment.
note in package_instnace.py we still catch RuntimeError which is outdated?
if we don't catch OSError and TimeoutExpired in run_and_log_subprocess() and rethrow with RuntimeError, i believe we want to catch those two in package_instnace.py
There was a problem hiding this comment.
You're right, we don't need to catch RuntimeError in package_instance.py anymore. I've removed that check.
For our purposes here, I feel that we don't need to catch OSError or TimeoutExpired, because either of those errors would indicate conditions or circumstances that are beyond the scope of the testing system. We can just let them be handled automatically; for example, I just forced a timeout in a testing subprocess by setting timeout=0.1, and the timeout was reported to the user in the ERROR summary at the end of the test.
What do you think?
…gration tests. (y-scope#1802) Co-authored-by: Jack Luo <jack.luo@yscope.com> Co-authored-by: Junhao Liao <junhao.liao@yscope.com>
3 participants
Description
This PR modifies integration test run output. It provides a minimal amount of information via the command line, and stores more verbose information in a test log directory (unique; one per test run). The output of each subprocess is stored within its own file in said directory.
Checklist
breaking change.
Validation performed
Ran
uv run pytest -m 'package'; all tests pass and information is reported accurately.Summary by CodeRabbit
Tests
Chores