-
Notifications
You must be signed in to change notification settings - Fork 88
feat(integration-tests): Clean up logging and error reporting in integration tests. #1802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1269402
9dbf299
e985fdf
7b4a68b
6fe0796
0562f48
2080fd5
6da4264
f5ec1b5
c0c4fd4
d9dc252
43f65aa
1c8992e
185184f
72a95d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,27 @@ | ||
| [pytest] | ||
| addopts = | ||
| --capture=no | ||
| --code-highlight=yes | ||
| --color=yes | ||
| -rA | ||
| --show-capture=no | ||
| --strict-config | ||
| --strict-markers | ||
| --verbose | ||
| --tb=short | ||
|
|
||
| env = | ||
| D:CLP_BUILD_DIR=../build | ||
| D:CLP_CORE_BINS_DIR=../build/core | ||
| D:CLP_PACKAGE_DIR=../build/clp-package | ||
|
|
||
| log_cli = True | ||
| 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 = %(asctime)s.%(msecs)03d %(levelname)s %(message)s | ||
| log_cli_date_format = %Y-%m-%d %H:%M:%S | ||
|
|
||
| log_file_mode = a | ||
| log_file_level = DEBUG | ||
| log_file_format = %(asctime)s.%(msecs)03d %(levelname)s [%(name)s:%(filename)s:%(lineno)d]: %(message)s | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. continuing from some of our offline discussion - this format is not consistent the loggers config used by the rest of the project. that said, i'm fine keeping whatever's been proposed like you said it improves readability in the consoles. in the long run, we should add infra for test CIs, where we should allow picking a JSON logger / some format string that's compatible with GH actions' problem matchers https://github.com/actions/toolkit/blob/main/docs/problem-matchers.md instead
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed |
||
| log_file_date_format = %Y-%m-%d %H:%M:%S | ||
|
|
||
| markers = | ||
| clp: mark tests that use the CLP storage engine | ||
| clp_json: mark tests that use the clp-json package | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,9 @@ show_error_end = true | |
| [tool.ruff] | ||
| extend = "../tools/yscope-dev-utils/exports/lint-configs/python/ruff.toml" | ||
|
|
||
| [tool.ruff.lint] | ||
| ignore = ["TRY400"] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we avoid this global override and if really needed use inline
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| [tool.uv] | ||
| default-groups = ["clp", "dev"] | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| """Fixtures that start and stop CLP package instances for integration tests.""" | ||
|
|
||
| import logging | ||
| from collections.abc import Iterator | ||
|
|
||
| import pytest | ||
|
|
@@ -13,9 +14,13 @@ | |
| stop_clp_package, | ||
| ) | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def fixt_package_instance(fixt_package_test_config: PackageTestConfig) -> Iterator[PackageInstance]: | ||
| def fixt_package_instance( | ||
| request: pytest.FixtureRequest, fixt_package_test_config: PackageTestConfig | ||
| ) -> Iterator[PackageInstance]: | ||
| """ | ||
| Starts a CLP package instance for the given configuration and stops it during teardown. | ||
|
|
||
|
|
@@ -25,18 +30,14 @@ def fixt_package_instance(fixt_package_test_config: PackageTestConfig) -> Iterat | |
| :param fixt_package_test_config: | ||
| :return: Iterator that yields the running package instance. | ||
| """ | ||
| mode_config = fixt_package_test_config.mode_config | ||
| mode_name = mode_config.mode_name | ||
|
|
||
| try: | ||
| start_clp_package(fixt_package_test_config) | ||
| logger.info("Starting the '%s' package...", mode_name) | ||
| start_clp_package(request, fixt_package_test_config) | ||
| instance = PackageInstance(package_test_config=fixt_package_test_config) | ||
| yield instance | ||
| except RuntimeError: | ||
| mode_config = fixt_package_test_config.mode_config | ||
| mode_name = mode_config.mode_name | ||
| base_port = fixt_package_test_config.base_port | ||
| pytest.fail( | ||
| f"Failed to start the {mode_name} package. This could mean that one of the ports" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe i wasn't reading carefully - why we removed this?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because |
||
| f" derived from base_port={base_port} was unavailable. You can specify a new value for" | ||
| " base_port with the '--base-port' flag." | ||
| ) | ||
| finally: | ||
| stop_clp_package(fixt_package_test_config) | ||
| logger.info("Stopping the '%s' package...", mode_name) | ||
| stop_clp_package(request, fixt_package_test_config) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |
|
|
||
| import pytest | ||
|
|
||
| from tests.utils.asserting_utils import run_and_assert | ||
| from tests.utils.asserting_utils import run_and_log_to_file | ||
| from tests.utils.config import ( | ||
| ClpCorePathConfig, | ||
| CompressionTestPathConfig, | ||
|
|
@@ -73,10 +73,10 @@ def test_clp_identity_transform( | |
| src_path, | ||
| ] | ||
| # fmt: on | ||
| run_and_assert(compression_cmd) | ||
| run_and_log_to_file(request, compression_cmd) | ||
|
|
||
| decompression_cmd = [bin_path, "x", compression_path, decompression_path] | ||
| run_and_assert(decompression_cmd) | ||
| run_and_log_to_file(request, decompression_cmd) | ||
|
|
||
| input_path = test_paths.logs_source_dir | ||
| output_path = test_paths.decompression_dir | ||
|
|
@@ -113,7 +113,7 @@ def test_clp_s_identity_transform( | |
| logs_source_dir=integration_test_logs.extraction_dir, | ||
| integration_test_path_config=integration_test_path_config, | ||
| ) | ||
| _clp_s_compress_and_decompress(clp_core_path_config, test_paths) | ||
| _clp_s_compress_and_decompress(request, clp_core_path_config, test_paths) | ||
|
|
||
| # Recompress the decompressed output that's consolidated into a single json file, and decompress | ||
| # it again to verify consistency. The compression input of the second iteration points to the | ||
|
|
@@ -126,7 +126,7 @@ def test_clp_s_identity_transform( | |
| logs_source_dir=test_paths.decompression_dir, | ||
| integration_test_path_config=integration_test_path_config, | ||
| ) | ||
| _clp_s_compress_and_decompress(clp_core_path_config, consolidated_json_test_paths) | ||
| _clp_s_compress_and_decompress(request, clp_core_path_config, consolidated_json_test_paths) | ||
|
|
||
| _consolidated_json_file_name = "original" | ||
| input_path = consolidated_json_test_paths.logs_source_dir / _consolidated_json_file_name | ||
|
|
@@ -140,6 +140,7 @@ def test_clp_s_identity_transform( | |
|
|
||
|
|
||
| def _clp_s_compress_and_decompress( | ||
| request: pytest.FixtureRequest, | ||
| clp_core_path_config: ClpCorePathConfig, | ||
| test_paths: CompressionTestPathConfig, | ||
| ) -> None: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just found this guy does have a docstring. shall we add one or was it too trivial?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should probably have one; maybe once you approve of the overarching approach of this PR then I'll go through the rest of the code and change the logging/docstrings (I stuck to just the package testing code for now) |
||
|
|
@@ -148,5 +149,6 @@ def _clp_s_compress_and_decompress( | |
| src_path = str(test_paths.logs_source_dir) | ||
| compression_path = str(test_paths.compression_dir) | ||
| decompression_path = str(test_paths.decompression_dir) | ||
| run_and_assert([bin_path, "c", compression_path, src_path]) | ||
| run_and_assert([bin_path, "x", compression_path, decompression_path]) | ||
|
|
||
| run_and_log_to_file(request, [bin_path, "c", compression_path, src_path]) | ||
| run_and_log_to_file(request, [bin_path, "x", compression_path, decompression_path]) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of appending to the same file, can isolate the logs from different test runs instead? e.g., use
set_log_pathwith the defaultlog_file_mode = w. see the test code at pytest-dev/pytest#4752 for exampleThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
It seems better to me to have everything written to the log file in order, so that it looks like this:
The logs from different test runs are already isolated by the code in
conftest.py.