cmake ctest support #6
Conversation
WalkthroughThe Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 16
Configuration used: CodeRabbit UI
Files selected for processing (13)
- phlop/app/init.py (1 hunks)
- phlop/app/cmake.py (1 hunks)
- phlop/app/git.py (1 hunks)
- phlop/app/perf.py (1 hunks)
- phlop/os.py (2 hunks)
- phlop/proc.py (3 hunks)
- phlop/reflection.py (1 hunks)
- phlop/run/test_cases.py (2 hunks)
- phlop/string.py (1 hunks)
- phlop/testing/parallel_processor.py (3 hunks)
- phlop/testing/test_cases.py (1 hunks)
- sh/lint.sh (1 hunks)
- tests/all_concurrent.py (1 hunks)
Additional comments: 30
phlop/app/__init__.py (1)
- 1-1: The hunk shows an empty file, which aligns with the summary stating that several functions have been removed. Ensure that the removal of these functions is intentional and that their responsibilities are covered elsewhere in the codebase.
phlop/app/cmake.py (5)
8-9: The
versionfunction is empty and has no implementation. Please verify if this is intentional or if the implementation is missing.20-22: The
configfunction correctly uses themake_config_strfunction to construct the command and then runs it. Ensure that theextraparameter is handled safely as mentioned in the previous comment.25-26: The
buildfunction correctly chooses between "ninja" and "make" based on theuse_ninjaparameter.70-87: The
list_testsfunction correctly constructs the command, runs it, and parses the output intoCTest_testinstances. Ensure that thebuild_dirparameter is properly sanitized to prevent command injection vulnerabilities.90-94: The
test_cmdfunction correctly generates a test command based on the provided test name and verbosity.phlop/app/git.py (1)
- 29-32: The
checkoutfunction has been updated to remove therecreatelogic and now only creates a branch if it does not exist andcreateisTrue. Ensure that all parts of the codebase that depended on therecreatefunctionality have been updated to handle this change.phlop/os.py (2)
3-3: The import of
Pathfrompathlibis correctly added to support the newwrite_to_filefunction.24-25: The addition of a check for the existence of
new_cwdbefore changing the directory is a good practice for error handling and robustness.phlop/reflection.py (5)
3-3: The addition of the
loggingmodule is appropriate for the new error logging functionality.9-9: The addition of the
fail_on_import_errorparameter allows for better control over the function's behavior in case of import errors, enhancing the robustness of the function.27-30: The exception handling for
ModuleNotFoundErroris well-implemented, providing a choice between raising the exception or logging an error based on thefail_on_import_errorflag.30-30: Logging the error message when a
ModuleNotFoundErroroccurs is a good practice for debugging and understanding why a module failed to import.16-32: The modification of
sys.pathis correctly handled within atryblock, and the original path is restored in thefinallyblock, which is good practice to avoid side effects on the module search path.phlop/run/test_cases.py (3)
14-21: The addition of the
--cmakeflag and--filteroption to the argument parser is correctly implemented. Ensure that the help descriptions are added for these new arguments for better user guidance.40-45: The function
get_test_casesis correctly refactored to handle the new--cmakeflag. Ensure that thepp.load_cmake_testsfunction is implemented and correctly handles the loading of CMake tests.51-53: The refactored call to
pp.processcorrectly passes then_coresandprint_onlyarguments. Ensure that thepp.processfunction signature is updated to accept these named parameters.phlop/testing/parallel_processor.py (4)
12-30: The
CallableTestclass has been refactored to use atest_caseobject, which is a good approach for encapsulating test case data. Ensure that all instances whereCallableTestis instantiated have been updated to pass atest_caseobject.45-48: The
print_testsfunction correctly accesses thecmdattribute from thetestobject. This change aligns with the new structure of test cases.51-55: The early return in the
processfunction when there are no tests to run is a good practice to avoid unnecessary processing.103-110: The
waiterfunction correctly handles the test case execution status and prints the command and status. It also properly manages the core availability and increments the finished counter for each batch. This is a good use of resources and provides clear feedback on test execution.phlop/testing/test_cases.py (8)
14-19: The
TestCasedataclass is well-defined with fields for command, environment, working directory, and log file path. This should facilitate the representation of test cases.22-26: The
TestBatchclass is a simple encapsulation for a batch of test cases, which should be useful for handling multiple test cases together.28-39: The
DefaultTestCaseExtractorclass returns a list with a singleTestCaseinstance. Ensure this aligns with the intended use case, as it may need to handle multiple test cases if a single CTest test can correspond to more than oneTestCase.64-68: The
EXTRACTORSlist is correctly populated with instances of the test case extractor classes. This should allow for easy extension with additional extractors in the future.71-72: The
python3_default_test_cmdfunction is a concise way to generate a default test command for Python 3 unit tests.75-90: The
load_test_cases_infunction correctly loads test cases from classes and allows for an optional test command function and log file path. This should provide flexibility in test case loading.93-105: The
load_test_cases_from_cmakefunction modifies thePYTHONPATHto include the test's working directory, which is important for Python tests to locate necessary modules. This seems to be a well-thought-out approach.112-133: The
load_cmake_testsfunction correctly loads CMake tests as a batch and uses theEXTRACTORSlist to process each test. This should allow for flexible and extensible test case extraction.tests/all_concurrent.py (1)
- 10-26: The changes to the import statement and function calls are consistent with the refactoring described in the pull request summary. Ensure that the
pp.TestBatchclass and thepp.load_test_cases_infunction are correctly defined in theparallel_processormodule. The code is clean and follows best practices.
| @@ -7,8 +7,13 @@ set -e | |||
|
|
|||
| PY_FILES=$(find . -name "*.py") | |||
There was a problem hiding this comment.
The PY_FILES variable is no longer necessary since black and isort are applied to directories and not individual files. Consider removing this variable to clean up the script.
- PY_FILES=$(find . -name "*.py")Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| PY_FILES=$(find . -name "*.py") |
There was a problem hiding this comment.
autoflake is per file
There was a problem hiding this comment.
@PhilipDeegan You're correct, autoflake does require the file list for its operation. The PY_FILES variable should be retained for that purpose. Thank you for pointing that out.
If you found the review helpful, we would love if you give us a shout-out on X (twitter).
|
|
||
| PY_FILES=$(find . -name "*.py") | ||
|
|
||
| python3 -m black phlop tests | ||
| pylint --errors-only phlop tests | ||
| isort phlop tests | ||
|
|
||
| for FILE in ${PY_FILES[@]}; do | ||
| python3 -m black "$FILE" | ||
|
|
||
| autoflake -i "$FILE" | ||
| isort "$FILE" | ||
|
|
||
|
|
||
| done |
There was a problem hiding this comment.
The changes to the linting script are appropriate and align with the summary provided. The script now applies black, pylint, and isort to the entire directories, which is more efficient than the previous approach of processing individual files. However, the PY_FILES variable is now redundant.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files selected for processing (14)
- ISSUES.md (1 hunks)
- phlop/app/init.py (1 hunks)
- phlop/app/cmake.py (1 hunks)
- phlop/app/git.py (1 hunks)
- phlop/app/perf.py (1 hunks)
- phlop/os.py (2 hunks)
- phlop/proc.py (3 hunks)
- phlop/reflection.py (1 hunks)
- phlop/run/test_cases.py (2 hunks)
- phlop/string.py (1 hunks)
- phlop/testing/parallel_processor.py (3 hunks)
- phlop/testing/test_cases.py (1 hunks)
- sh/lint.sh (1 hunks)
- tests/all_concurrent.py (1 hunks)
Additional comments: 29
ISSUES.md (1)
- 5-6: The information provided in
ISSUES.mdis accurate and relevant for developers working with Python 3.8. It's important to note that the end of life for Python 3.8 is mentioned as October 2024, which is correct based on the knowledge cutoff date.phlop/app/__init__.py (1)
- 1-1: The hunk shows an empty file, which suggests that all previous content has been removed. If this is intentional and all functionality has been refactored into other modules, ensure that there are no dangling references to the removed functions in the rest of the codebase.
phlop/app/cmake.py (3)
42-42: Skip commenting on the
working_dirfield default value as this issue has been previously raised.70-87: The implementation of the
list_testsfunction to parse JSON and createCTest_testinstances looks correct.90-94: The implementation of the
test_cmdfunction to generate a test command based on the provided test name and verbosity looks correct.phlop/app/git.py (4)
26-32: The
checkoutfunction has been simplified to remove therecreatelogic. Ensure that all calls to this function have been updated to remove therecreateargument if it was previously used.26-32: The
checkoutfunction's logic is correct, but it's important to ensure that thecreateparameter is being used correctly in all calls to this function since therecreatefunctionality has been removed.26-32: The
checkoutfunction now directly checks if the branch exists and creates it if specified. This change simplifies the function and removes potentially dangerous logic that could delete and recreate branches.26-32:
phlop/app/perf.py (1)
- 12-12: The code will raise an AttributeError because lists do not have a
splitmethod. The string should be split by spaces first and then each part should be split by ".".- return [int(digit) for digit in proc.split(" ").split(".")] + return [int(digit) for part in proc.split(" ") for digit in part.split(".") if digit.isdigit()]phlop/proc.py (3)
34-35: The file descriptors for
stdoutandstderrare opened but not explicitly closed, which can lead to resource leaks. Consider using a context manager to ensure that files are properly closed.56-62: The
runmethod only catchessubprocess.CalledProcessError. Consider catching a more general exception (e.g.,Exception) to handle unexpected errors that may occur during the subprocess execution.124-125: The
binary_exists_on_pathfunction raises aValueErrorwith a message that seems to be a placeholder. Ensure that this is intentional and consider implementing a proper check or removing the placeholder if it's not needed.phlop/reflection.py (4)
3-3: The addition of the
loggingmodule is appropriate for the new logging functionality introduced in theclasses_in_filefunction.9-9: The addition of the
fail_on_import_errorparameter to theclasses_in_filefunction allows for better control over the function's behavior in case of import errors. Ensure that all calls to this function have been updated to handle the new parameter.27-30: The exception handling for
ModuleNotFoundErrorwith an option to log the error and continue is a good addition. Ensure that the logging level is set appropriately elsewhere in the application to capture these error messages.16-32: The modification and restoration of
sys.pathare handled correctly within atry-finallyblock, ensuring that the original path is restored even if an exception occurs.phlop/run/test_cases.py (4)
1-2: The new imports of
multiprocessingandosare used in the code, which is good for modularity and maintainability.14-18: The addition of the
--cmakeflag and--filteroption to the argument parser is consistent with the summary indicating support for CMake and test filtering.40-45: The
get_test_casesfunction has been correctly updated to handle the--cmakeflag, which aligns with the summary's mention of CMake support.51-52: The refactoring to use
pp.processandpp.load_test_cases_inis consistent with the changes in theparallel_processormodule structure.phlop/testing/parallel_processor.py (4)
13-28: The changes to the
CallableTestclass constructor and the__call__method to usetest_caseattributes are consistent with the summary and seem logically correct.45-48: The update to the
print_testsfunction to printtest.cmdis consistent with the summary and seems logically correct.51-55: The early return in the
processfunction when there are no tests to run is a good addition for performance, as it avoids unnecessary processing.103-107: The changes to the
waiterfunction to accesscmdattribute fromproc.test_caseand the handling of test completion status are consistent with the summary and seem logically correct.phlop/testing/test_cases.py (1)
- 1-133: The changes in the file seem to be correctly implemented with no apparent issues in the logic or structure of the code. The use of dataclasses, context managers, and default parameters are all good practices in Python.
sh/lint.sh (2)
8-8: The
PY_FILESvariable is no longer necessary and should be removed to clean up the script.14-19: The loop correctly applies
autoflaketo individual files, which is consistent with the summary provided.tests/all_concurrent.py (1)
- 10-26: The changes to the import statements and function calls are consistent with the new module structure and usage of the
parallel_processormodule. The use of the aliasppis clear and the updated function calls reflect this change. The refactoring to usepp.TestBatchin theprocessfunction call is also consistent with the summary provided.
| import time | ||
| import unittest | ||
| from multiprocessing import Process, Queue, cpu_count | ||
|
|
||
| from phlop.proc import ProcessNonZeroExitCode, run | ||
| from phlop.testing.test_cases import * | ||
|
|
||
|
|
||
| class TestCaseFailure(Exception): | ||
| ... | ||
|
|
||
|
|
||
| def test_cmd(clazz, test_id, cores): | ||
| return f"python3 -m {clazz.__module__} {clazz.__name__}.{test_id}" | ||
|
|
||
|
|
||
| class TestBatch: | ||
| def __init__(self, tests, cores=1): | ||
| self.tests = tests | ||
| self.cores = cores | ||
|
|
||
|
|
||
| def load_test_cases_in(classes, cores=1, test_cmd_fn=None): | ||
| test_cmd_fn = test_cmd_fn if test_cmd_fn else test_cmd | ||
|
|
||
| tests, loader = [], unittest.TestLoader() | ||
| for test_class in classes: | ||
| for suite in loader.loadTestsFromTestCase(test_class): | ||
| tests += [test_cmd_fn(type(suite), suite._testMethodName, cores)] | ||
|
|
||
| return TestBatch(tests, cores) | ||
|
|
||
|
|
||
| class CallableTest: | ||
| def __init__(self, batch_index, cmd): | ||
| def __init__(self, batch_index, test_case): | ||
| self.batch_index = batch_index | ||
| self.cmd = cmd | ||
| self.test_case = test_case | ||
| self.run = None | ||
|
|
||
| def __call__(self, **kwargs): | ||
| self.run = run( | ||
| self.cmd.split(), | ||
| self.test_case.cmd.split(), | ||
| shell=False, | ||
| capture_output=True, | ||
| check=True, | ||
| print_cmd=False, | ||
| env=self.test_case.env, | ||
| working_dir=self.test_case.working_dir, | ||
| log_file_path=self.test_case.log_file_path, | ||
| ) | ||
| if self.run.exitcode != 0: | ||
| print(self.run.stderr) |
There was a problem hiding this comment.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [31-31]
Typo in the CoreCount class attribute name.
- self.proces = []
+ self.procs = []| return True | ||
|
|
||
|
|
||
| def checkout(branch, create=False, recreate=False): | ||
| if recreate: | ||
| delete_branch(branch) | ||
| create = True | ||
|
|
||
| def checkout(branch, create=False): | ||
| if create and not branch_exists(branch): | ||
| run(f"git checkout -b {branch}", check=True) | ||
| else: |
There was a problem hiding this comment.
Consider using git rev-parse --verify instead of git show-branch for checking branch existence, as it is more commonly used and might be more performant.
def branch_exists(branch):
try:
- run(f"git show-branch {branch}", check=True)
+ run(f"git rev-parse --verify {branch}", check=True)
except subprocess.CalledProcessError as e:
return False # exit failure means branch does not exist
return TrueCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return True | |
| def checkout(branch, create=False, recreate=False): | |
| if recreate: | |
| delete_branch(branch) | |
| create = True | |
| def checkout(branch, create=False): | |
| if create and not branch_exists(branch): | |
| run(f"git checkout -b {branch}", check=True) | |
| else: | |
| return True | |
| def branch_exists(branch): | |
| try: | |
| run(f"git rev-parse --verify {branch}", check=True) | |
| except subprocess.CalledProcessError as e: | |
| return False # exit failure means branch does not exist | |
| return True | |
| def checkout(branch, create=False): | |
| if create and not branch_exists(branch): | |
| run(f"git checkout -b {branch}", check=True) | |
| else: |
| def make_config_str(path, cxx_flags=None, use_ninja=False, use_ccache=False, extra=""): | ||
| cxx_flags = "" if cxx_flags is None else f'-DCMAKE_CXX_FLAGS="{cxx_flags}"' | ||
| ccache = "" if use_ccache is False else "-DCMAKE_CXX_COMPILER_LAUNCHER=ccache" | ||
| ninja = "" if not use_ninja else "-G Ninja" | ||
|
|
||
| return f"cmake {path} {cxx_flags} {ninja} {ccache} {extra}" |
There was a problem hiding this comment.
The extra parameter is directly interpolated into the command string. Ensure that any input passed to this parameter is properly sanitized to prevent command injection vulnerabilities.
PhilipDeegan
force-pushed
the
cmake
branch
2 times, most recently
from
1ff2752
to
61bdc85
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (14)
- ISSUES.md (1 hunks)
- phlop/app/init.py (1 hunks)
- phlop/app/cmake.py (1 hunks)
- phlop/app/git.py (1 hunks)
- phlop/app/perf.py (1 hunks)
- phlop/os.py (2 hunks)
- phlop/proc.py (3 hunks)
- phlop/reflection.py (1 hunks)
- phlop/run/test_cases.py (2 hunks)
- phlop/string.py (1 hunks)
- phlop/testing/parallel_processor.py (3 hunks)
- phlop/testing/test_cases.py (1 hunks)
- sh/lint.sh (1 hunks)
- tests/all_concurrent.py (1 hunks)
Files skipped from review due to trivial changes (1)
- sh/lint.sh
Additional comments: 37
ISSUES.md (1)
- 5-6: The information provided is accurate and relevant for developers working with Python 3.8. It's important to ensure that the codebase does not use features that are not supported in Python 3.8 until the project drops support for this version.
phlop/app/__init__.py (1)
- 1-1: The hunk is empty, indicating that there may have been a removal of code. Ensure that any code that depended on the removed functions has been updated or refactored accordingly.
phlop/app/cmake.py (3)
- 20-21: The
extraparameter is not being passed tomake_config_strfrom theconfigfunction. This should be corrected to ensure that theextraparameter is used when generating the command string.- cmd = make_config_str(path, cxx_flags, use_ninja, extra) + cmd = make_config_str(path, cxx_flags, use_ninja, use_ccache, extra)
42-42: The
working_dirfield has a default factory that returnsNone, which may lead to type inconsistencies. Consider changing the default to an empty string or handlingNonevalues appropriately whereworking_diris used.12-17: The
extraparameter is directly interpolated into the command string. Ensure that any input passed to this parameter is properly sanitized to prevent command injection vulnerabilities.phlop/app/git.py (2)
26-32: The
checkoutfunction has been correctly updated to remove therecreatelogic, and now it only handles branch creation if thecreateflag is set and the branch does not exist. Ensure that all calls to this function have been updated to remove anyrecreateparameter that may have been passed previously.26-26: Skip generating a comment about the
branch_existsfunction as the previous comment is still valid.phlop/app/perf.py (2)
9-12: The change in the return statement assumes that the version number is always at the end of the output. Verify that this assumption holds true for all versions of
perfthat the application supports. If the output format ofperf -vis not consistent across versions, this could lead to incorrect parsing of the version number.6-15: The rest of the code in the provided file appears to be unchanged and is not part of the review.
phlop/os.py (4)
22-22:
35-39:
24-25: The addition of a check for the existence of
new_cwdbefore changing the directory is a good practice for error handling and ensures that the function fails early if the directory does not exist.37-39: The addition of creating parent directories if they do not exist before writing to the file is a useful feature that enhances the robustness of the
write_to_filefunction.phlop/proc.py (4)
- 17-17: The
envparameter should not be initialized with a mutable default argument (empty dict). UseNoneand then initialize it inside the method.- env: dict = {}, # dict[str, str] # eventually + env: dict = None, # dict[str, str] # eventually + if env is None: + env = {}
- 34-35: The file descriptors for stdout and stderr are opened but not explicitly closed, which can lead to resource leaks. Consider using a context manager to ensure that files are properly closed.
- stdout=open(f"{log_file_path}.stdout", "w"), - stderr=open(f"{log_file_path}.stderr", "w"), + stdout=open(f"{log_file_path}.stdout", "w").__enter__(), + stderr=open(f"{log_file_path}.stderr", "w").__enter__(),
56-62: The
runmethod only catchessubprocess.CalledProcessError. Consider catching a more general exception (e.g.,Exception) to handle unexpected errors that may occur during the subprocess execution.124-125: The
binary_exists_on_pathfunction raises aValueErrorwith a message that seems to be a placeholder. Ensure that this is intentional and consider implementing a proper check or removing the placeholder if it's not needed.phlop/reflection.py (4)
3-3: The addition of the
loggingmodule is appropriate for the new error logging functionality.9-9: Ensure that all calls to
classes_in_filethroughout the codebase are updated to handle the newfail_on_import_errorparameter.27-30: The error handling and logging for
ModuleNotFoundErrorare implemented correctly. Verify that the logging level is appropriately set elsewhere in the application to capture this error message.16-32: The use of a try-finally block to ensure
sys.pathis restored to its original state is good practice and prevents potential side effects on the module import system.phlop/run/test_cases.py (5)
1-2: The new imports
multiprocessingandosare used appropriately in the file.14-21: The argument parser has been updated with new arguments and defaults. Ensure that all new arguments are documented and used correctly throughout the codebase.
27-36: The
verify_cli_argsfunction has been updated to handle the--dirargument and check for directory existence. Ensure that the changes are compatible with the rest of the codebase and that the error messages are clear and informative.40-45: The
get_test_casesfunction has been updated to handle CMake tests. Ensure that the new functionality is tested and that the function integrates well with the existing testing framework.48-53: The
mainfunction has been updated to incorporate the new argument verification and test case retrieval logic. Ensure that the changes are consistent with the expected program flow and that the new features are properly integrated.phlop/string.py (1)
- 1-6: The function now handles strings and bytes correctly and raises a ValueError for unknown types. However, consider if ignoring errors during decoding is the desired behavior, as it may lead to data loss or unexpected results.
phlop/testing/parallel_processor.py (1)
- 12-30: The changes to the
CallableTestclass to storetest_caseinstead ofcmdand the corresponding updates to the__call__method are consistent with the summary and appear to be correctly implemented.phlop/testing/test_cases.py (8)
14-19: The
TestCasedataclass is well-defined and includes default factories for optional fields.22-26: The
TestBatchclass is a straightforward container for test cases and the number of cores, which is good for batch processing.28-38: The
DefaultTestCaseExtractorclass correctly creates aTestCaseinstance, but ensure that the log file path concatenation handles edge cases, such as when_LOG_DIRis the root directory.64-68: The
EXTRACTORSlist is correctly populated with instances of the test case extractor classes.71-72: The
python3_default_test_cmdfunction is a simple and effective way to generate a default test command for Python 3 test cases.75-90: The
load_test_cases_infunction is well-implemented, providing a way to load test cases from classes and allowing for a custom test command function.93-105: The
load_test_cases_from_cmakefunction correctly sets up the environment and loads test cases from a CMake test, using theclasses_in_filefunction to find test classes.108-129: The
load_cmake_testsfunction is implemented to load CMake tests and extract test cases using the defined extractors. It returns aTestBatchwhich is good for batch processing.tests/all_concurrent.py (1)
- 10-26: The changes to the import statements and function calls are consistent with the summary provided. The use of the
ppalias forparallel_processoris clear and the updated directory path for locating test classes is correct.
| fail += proc.run.exitcode | ||
| if fail_fast and fail > 0: | ||
| raise TestCaseFailure("Some tests have failed") | ||
| print(proc.cmd, f"{status} in {proc.run.run_time:.2f} seconds") | ||
| print( | ||
| proc.test_case.cmd, f"{status} in {proc.run.run_time:.2f} seconds" |
There was a problem hiding this comment.
The waiter function should be updated to access the cmd attribute from the test_case attribute of the proc object, as the direct cmd attribute on proc no longer exists.
- proc.test_case.cmd, f"{status} in {proc.run.run_time:.2f} seconds"
+ proc.test_case.test_case.cmd, f"{status} in {proc.run.run_time:.2f} seconds"Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| fail += proc.run.exitcode | |
| if fail_fast and fail > 0: | |
| raise TestCaseFailure("Some tests have failed") | |
| print(proc.cmd, f"{status} in {proc.run.run_time:.2f} seconds") | |
| print( | |
| proc.test_case.cmd, f"{status} in {proc.run.run_time:.2f} seconds" | |
| fail += proc.run.exitcode | |
| if fail_fast and fail > 0: | |
| raise TestCaseFailure("Some tests have failed") | |
| print( | |
| proc.test_case.test_case.cmd, f"{status} in {proc.run.run_time:.2f} seconds" |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- pyproject.toml
Files selected for processing (7)
- phlop/main.py (1 hunks)
- phlop/app/main.py (1 hunks)
- phlop/os.py (2 hunks)
- phlop/run/main.py (1 hunks)
- phlop/run/test_cases.py (1 hunks)
- phlop/testing/main.py (1 hunks)
- setup.py (1 hunks)
Files skipped from review due to trivial changes (3)
- phlop/app/main.py
- phlop/run/main.py
- setup.py
Additional comments: 7
phlop/__main__.py (1)
- 1-14: The list of modules printed by
_STR_should be verified to ensure it is up-to-date with the modules currently available in the project, especially after the refactor mentioned in the PR objectives and AI-generated overview.Verification successful
The list of modules in
_STR_withinphlop/__main__.pyis consistent with the current structure of thephlopdirectory. There are no new top-level modules missing from the list, and the submodules withinphlop.app,phlop.run, andphlop.testingdo not need to be listed separately. The list is up-to-date with the modules available in the project.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # List all Python modules in the phlop directory to verify against the _STR_ variable. fd --extension py --base-directory phlopLength of output: 295
phlop/os.py (2)
2-3: The addition of
contextlibandPathimports aligns with the new functionality introduced in thepushdandwrite_to_filefunctions.22-23: The added directory existence check in the
pushdfunction is a good practice to prevent runtime errors. Ensure that the error message is clear and actionable for the user.phlop/run/test_cases.py (4)
10-34: The
cli_args_parserfunction has been updated with new arguments and default values, which aligns with the PR objectives. Ensure that these new arguments are correctly utilized in the rest of the codebase.40-40: The conversion of
cli_args.coresto an integer is redundant since the--coresargument is already defined as an integer type in the argument parser. This should be removed to avoid unnecessary code.- cli_args.cores = int(cli_args.cores)
51-56: The
get_test_casesfunction has been introduced with conditional logic based on thecmakeflag. This change should be cross-verified with the rest of the codebase to ensure that it integrates properly and that thecmakeflag is being handled as expected.59-70: The
mainfunction has been refactored to utilize the new argument parsing and processing functions. Ensure that the error handling is appropriate and that the parsed arguments are correctly handled throughout the function.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- pyproject.toml
Files selected for processing (7)
- phlop/main.py (1 hunks)
- phlop/app/main.py (1 hunks)
- phlop/os.py (2 hunks)
- phlop/run/main.py (1 hunks)
- phlop/run/test_cases.py (1 hunks)
- phlop/testing/main.py (1 hunks)
- setup.py (1 hunks)
Files skipped from review due to trivial changes (3)
- phlop/main.py
- phlop/app/main.py
- setup.py
Files skipped from review as they are similar to previous changes (4)
- phlop/os.py
- phlop/run/main.py
- phlop/run/test_cases.py
- phlop/testing/main.py
There was a problem hiding this comment.
Review Status
Actionable comments generated: 14
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- pyproject.toml
Files selected for processing (10)
- phlop/main.py (1 hunks)
- phlop/app/main.py (1 hunks)
- phlop/app/cmake.py (1 hunks)
- phlop/dict.py (1 hunks)
- phlop/os.py (2 hunks)
- phlop/run/main.py (1 hunks)
- phlop/run/test_cases.py (1 hunks)
- phlop/testing/main.py (1 hunks)
- phlop/testing/test_cases.py (1 hunks)
- setup.py (1 hunks)
Files skipped from review due to trivial changes (3)
- phlop/main.py
- phlop/app/main.py
- setup.py
Files skipped from review as they are similar to previous changes (3)
- phlop/os.py
- phlop/run/main.py
- phlop/testing/main.py
Additional comments: 2
phlop/app/cmake.py (1)
- 8-9: The
versionfunction is empty. Please verify if this is intentional or if the implementation is missing.phlop/run/test_cases.py (1)
- 12-12: The function
parse_cli_argshas been renamed tocli_args_parser. Verify that all references to the old function name have been updated across the codebase.Verification successful
The search for the old function name
parse_cli_argsacross the codebase did not yield any results. This suggests that there are no remaining references to the old function name, which aligns with the changes made in the pull request.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the old function name usage across the codebase. Expecting no results. rg --type python 'parse_cli_args\('Length of output: 68
Script:
#!/bin/bash # Search for the old function name usage across the codebase without specifying the file type. rg 'parse_cli_args\('Length of output: 23
| class ValDict(dict): | ||
| def __init__(self, **d): | ||
| for k, v in d.items(): | ||
| object.__setattr__(self, k, v) |
There was a problem hiding this comment.
The ValDict class sets dictionary keys as attributes, which could lead to conflicts if keys overlap with dict method names. Consider adding a check to prevent setting attributes that conflict with dict methods or built-in attributes.
The ValDict class does not handle updates to the dictionary after initialization. Consider implementing __setitem__ and __delitem__ to synchronize changes to the dictionary with the instance attributes.
The ValDict class does not override the __getattr__ method, which means accessing non-existent attributes that correspond to dictionary keys will not return the dictionary value. Consider implementing __getattr__ to provide consistent access to dictionary items as attributes.
| def __init__(self, **d): | ||
| for k, v in d.items(): | ||
| object.__setattr__(self, k, v) |
There was a problem hiding this comment.
The __init__ method lacks error handling for invalid key names that cannot be set as attributes. Consider adding validation for key names to ensure they are valid attribute identifiers.
| working_dir=ctest_test.working_dir, | ||
| log_file_path=_LOG_DIR | ||
| / ".phlop" | ||
| / f"{Path(ctest_test.working_dir).relative_to(_LOG_DIR)}", |
There was a problem hiding this comment.
Potential issue with path operations.
- log_file_path=_LOG_DIR
- / ".phlop"
- / f"{Path(ctest_test.working_dir).relative_to(_LOG_DIR)}",Ensure that ctest_test.working_dir is always a subdirectory of _LOG_DIR to avoid ValueError from Path.relative_to(). If not, consider adding error handling or validation.
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| working_dir=ctest_test.working_dir, | |
| log_file_path=_LOG_DIR | |
| / ".phlop" | |
| / f"{Path(ctest_test.working_dir).relative_to(_LOG_DIR)}", | |
| working_dir=ctest_test.working_dir, |
| class GoogleTestCaseExtractor: | ||
| def __call__(self, ctest_test): | ||
| ... | ||
| # not configured, assumed fast per file | ||
| # print("GoogleTestCaseExtractor") | ||
| # exec binary with `--gtest_list_tests` and see if it doesn't fail | ||
| # p = run( | ||
| # ctest_test.cmd + " --gtest_list_tests --gtest_output=json:gtest.json", | ||
| # working_dir=ctest_test.working_dir, | ||
| # ) | ||
| # print(p.stdout) | ||
|
|
There was a problem hiding this comment.
Incomplete implementation or dead code.
- ...
- # not configured, assumed fast per file
- # print("GoogleTestCaseExtractor")
- # exec binary with `--gtest_list_tests` and see if it doesn't fail
- # p = run(
- # ctest_test.cmd + " --gtest_list_tests --gtest_output=json:gtest.json",
- # working_dir=ctest_test.working_dir,
- # )
- # print(p.stdout)Clarify the status of GoogleTestCaseExtractor. If this is not intended for use, consider removing the commented-out code to avoid confusion.
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| class GoogleTestCaseExtractor: | |
| def __call__(self, ctest_test): | |
| ... | |
| # not configured, assumed fast per file | |
| # print("GoogleTestCaseExtractor") | |
| # exec binary with `--gtest_list_tests` and see if it doesn't fail | |
| # p = run( | |
| # ctest_test.cmd + " --gtest_list_tests --gtest_output=json:gtest.json", | |
| # working_dir=ctest_test.working_dir, | |
| # ) | |
| # print(p.stdout) | |
| class GoogleTestCaseExtractor: | |
| def __call__(self, ctest_test): |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- pyproject.toml
Files selected for processing (10)
- phlop/main.py (1 hunks)
- phlop/app/main.py (1 hunks)
- phlop/app/cmake.py (1 hunks)
- phlop/dict.py (1 hunks)
- phlop/os.py (2 hunks)
- phlop/run/main.py (1 hunks)
- phlop/run/test_cases.py (1 hunks)
- phlop/testing/main.py (1 hunks)
- phlop/testing/test_cases.py (1 hunks)
- setup.py (1 hunks)
Files skipped from review due to trivial changes (6)
- phlop/main.py
- phlop/app/main.py
- phlop/app/cmake.py
- phlop/dict.py
- phlop/run/main.py
- setup.py
Files skipped from review as they are similar to previous changes (2)
- phlop/testing/main.py
- phlop/testing/test_cases.py
Additional comments: 6
phlop/os.py (2)
22-23: The addition of a directory existence check in the
pushdfunction is a good practice to prevent runtime errors. Ensure that the function's documentation is updated to reflect this new behavior.3-3: The import of
Pathfrompathlibis appropriate for the file operations performed in thewrite_to_filefunction.phlop/run/test_cases.py (4)
- 11-11: The function
parse_cli_argshas been renamed tocli_args_parser. Verify that all references to this function have been updated accordingly.Verification successful
The shell script results indicate that the old function name
parse_cli_argsis no longer present in any Python files within the codebase. The new function namecli_args_parseris found inphlop/run/test_cases.py, which is consistent with the changes described in the PR. There are no references to the old function name, suggesting that the necessary updates to function calls have been made.* 48-59: The `get_test_cases` function has been updated to handle CMake test cases when the `cmake` flag is set. Ensure that the `pp.load_cmake_tests` and `pp.TestBatch` functions are implemented correctly and handle the new arguments.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the old function name to ensure it's no longer used. rg --type python 'parse_cli_args' # Search for the new function name to ensure it's being used correctly. rg --type python 'cli_args_parser'Length of output: 130
Script:
#!/bin/bash # Search for the old function name to ensure it's no longer used. rg 'parse_cli_args' --glob '*.py' # Search for the new function name to ensure it's being used correctly. rg 'cli_args_parser' --glob '*.py'Length of output: 170
Verification successful
The
rg(ripgrep) search has successfully located theload_cmake_testsfunction and theTestBatchclass within a file namedphlop/testing/test_cases.py. This confirms the existence of both entities in the codebase, which aligns with the changes mentioned in the PR review.Based on this information, it can be concluded that the
get_test_casesfunction inphlop/run/test_cases.pyis likely to be correctly implemented, as it references theload_cmake_testsfunction and theTestBatchclass which do exist in the codebase.Given this confirmation, the original approval with a verification request can be upheld, and no further action is needed regarding the implementation of these entities.
* 62-82: The `main` function has been refactored to use the new `cli_args_parser` and `verify_cli_args` functions. Ensure that the refactoring does not introduce any issues with how arguments are parsed and processed.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of pp.load_cmake_tests function. ast-grep --lang python --pattern $'def load_cmake_tests($_, $_, $_)' # Verify the implementation of pp.TestBatch class. ast-grep --lang python --pattern $'class TestBatch { $$$ }'Length of output: 921
Script:
#!/bin/bash # Simplify the pattern for the load_cmake_tests function to increase the chance of a match. ast-grep --lang python --pattern $'def load_cmake_tests($_, $_, $_)' # Correct the pattern syntax for the TestBatch class to match the class definition. ast-grep --lang python --pattern $'class TestBatch:'Length of output: 468
Script:
#!/bin/bash # Use ripgrep to search for the load_cmake_tests function and TestBatch class within Python files. rg --type py 'def load_cmake_tests' rg --type py 'class TestBatch'Length of output: 220
Verification successful
The output from the
rgcommand provides the context needed to verify the usage ofcli_args_parserandverify_cli_argswithin themainfunction of thephlop/run/test_cases.pyfile. The code snippet shows thatcli_args_parseris defined and returns a parser object. In themainfunction, this parser object is used to parse the command-line arguments, and the resultingcli_argsis then passed toverify_cli_argsfor further verification.The sequence of operations in the
mainfunction is as follows:
parser = cli_args_parser()initializes the argument parser.cli_args = verify_cli_args(parser.parse_args())parses the command-line arguments and then verifies them.This sequence matches the intended refactoring described in the review comment and the AI-generated summary of changes. There are no apparent issues with how arguments are parsed and processed based on the provided code snippet.
* 80-82: Catching all exceptions is not recommended as it can obscure the actual error. Consider catching specific exceptions to provide more detailed error information and handling.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct usage of cli_args_parser and verify_cli_args within the main function. ast-grep --lang python --pattern $'def main() { $$$ cli_args_parser($_) $$$ verify_cli_args($_) $$$ }'Length of output: 458
Script:
#!/bin/bash # Refine the pattern to correctly match the usage of cli_args_parser and verify_cli_args within the main function. ast-grep --lang python --pattern $'def main() { $_ = cli_args_parser() $$$ $_ = verify_cli_args($_.parse_args()) $$$ }'Length of output: 473
Script:
#!/bin/bash # Use ripgrep to search for the usage of cli_args_parser and verify_cli_args within the phlop/run/test_cases.py file. rg --context 5 'cli_args_parser|verify_cli_args' phlop/run/test_cases.pyLength of output: 1052
| def write_to_file(file, contents, mode="w", skip_if_empty=True): | ||
| if contents or not skip_if_empty: | ||
| try: | ||
| Path(file).parent.mkdir(parents=True, exist_ok=True) | ||
| with open(file, mode) as f: | ||
| f.write(contents) | ||
| except IOError as e: | ||
| raise RuntimeError(f"Failed to write to file {file}: {e}") |
There was a problem hiding this comment.
The write_to_file function should be documented to explain its parameters and behavior, especially the skip_if_empty flag. Additionally, consider adding a check for the validity of the mode parameter to ensure it's a valid file opening mode.
+ """
+ Writes contents to a file.
+
+ Parameters:
+ - file: The file path where contents will be written.
+ - contents: The content to write to the file.
+ - mode: The mode in which the file is opened, e.g., 'w' for write.
+ - skip_if_empty: If True, writing is skipped when contents are empty.
+
+ Raises:
+ - RuntimeError: If the file cannot be written.
+ """Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def write_to_file(file, contents, mode="w", skip_if_empty=True): | |
| if contents or not skip_if_empty: | |
| try: | |
| Path(file).parent.mkdir(parents=True, exist_ok=True) | |
| with open(file, mode) as f: | |
| f.write(contents) | |
| except IOError as e: | |
| raise RuntimeError(f"Failed to write to file {file}: {e}") | |
| def write_to_file(file, contents, mode="w", skip_if_empty=True): | |
| """ | |
| Writes contents to a file. | |
| Parameters: | |
| - file: The file path where contents will be written. | |
| - contents: The content to write to the file. | |
| - mode: The mode in which the file is opened, e.g., 'w' for write. | |
| - skip_if_empty: If True, writing is skipped when contents are empty. | |
| Raises: | |
| - RuntimeError: If the file cannot be written. | |
| """ | |
| if contents or not skip_if_empty: | |
| try: | |
| Path(file).parent.mkdir(parents=True, exist_ok=True) | |
| with open(file, mode) as f: | |
| f.write(contents) | |
| except IOError as e: | |
| raise RuntimeError(f"Failed to write to file {file}: {e}") |
PhilipDeegan
force-pushed
the
cmake
branch
2 times, most recently
from
e9ecf51
to
7ab09ba
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- pyproject.toml
Files selected for processing (12)
- phlop/main.py (1 hunks)
- phlop/app/main.py (1 hunks)
- phlop/app/cmake.py (1 hunks)
- phlop/dict.py (1 hunks)
- phlop/logger.py (1 hunks)
- phlop/os.py (2 hunks)
- phlop/reflection.py (1 hunks)
- phlop/run/main.py (1 hunks)
- phlop/run/test_cases.py (1 hunks)
- phlop/testing/main.py (1 hunks)
- phlop/testing/test_cases.py (1 hunks)
- setup.py (1 hunks)
Files skipped from review due to trivial changes (5)
- phlop/main.py
- phlop/app/main.py
- phlop/dict.py
- phlop/run/main.py
- setup.py
Files skipped from review as they are similar to previous changes (4)
- phlop/app/cmake.py
- phlop/os.py
- phlop/testing/main.py
- phlop/testing/test_cases.py
Additional comments: 6
phlop/logger.py (1)
- 1-10: The changes in
phlop/logger.pydo not appear to be directly related to the PR's stated objectives of enhancing CMake and CTest integration. Ensure that these changes are intentional and relevant to the PR.phlop/reflection.py (2)
9-9: Ensure that documentation and tests are updated to reflect the new
fail_on_import_errorparameter in theclasses_in_filefunction.27-30: The new error handling behavior based on the
fail_on_import_errorflag should be clearly communicated to users. Additionally, ensure that logging is set up appropriately to capture the error messages when exceptions are not raised.phlop/run/test_cases.py (3)
20-42: The refactoring and addition of new command-line arguments in
cli_args_parserappear to be correctly implemented with appropriate help descriptions.57-68: The implementation of conditional logic in
get_test_casesbased on thecmakeflag is consistent with the PR's objectives to handle CMake test cases.80-82: Catching
SystemExitexceptions separately from other exceptions is a good practice, as it allows for different handling of normal exit scenarios versus unexpected exceptions.
phlop/reflection.py
Outdated
| old_path = sys.path | ||
| try: | ||
| sys.path += [os.getcwd()] | ||
| for name, cls in inspect.getmembers( | ||
| importlib.import_module(module), inspect.isclass | ||
| ): | ||
| should_add = subclasses_only == None or any( | ||
| [issubclass(cls, sub) for sub in subclasses_only] | ||
| ) | ||
| if should_add: | ||
| classes += [cls] | ||
| except (ValueError, ModuleNotFoundError) as e: | ||
| if fail_on_import_error: | ||
| raise e | ||
| logging.error(f"Skipping on error: {e} in module {module}") | ||
| finally: | ||
| sys.path = old_path |
There was a problem hiding this comment.
The modification of sys.path is a global operation that could have unintended side effects. It's correctly reverted in the finally block, but consider using a context manager to ensure sys.path is restored even if an unexpected exception occurs.
from contextlib import contextmanager
@contextmanager
def extend_sys_path(path):
old_path = sys.path[:]
sys.path.append(path)
try:
yield
finally:
sys.path = old_path
# Usage within the function
with extend_sys_path(os.getcwd()):
# ... import module and get classes ...There was a problem hiding this comment.
@PhilipDeegan, I'm glad you found the suggestion helpful! 😊
(\(\
( -.-)
o_(")(")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
phlop/reflection.py
Outdated
| ) | ||
| if should_add: | ||
| classes += [cls] | ||
| old_path = sys.path |
There was a problem hiding this comment.
The assignment old_path = sys.path should be changed to old_path = sys.path[:] to ensure that a copy of the original sys.path is made. This prevents the finally block from restoring sys.path to a modified state if it's changed elsewhere after the assignment.
- old_path = sys.path
+ old_path = sys.path[:]Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| old_path = sys.path | |
| old_path = sys.path[:] |
|
|
||
| import logging | ||
|
|
||
| logging.basicConfig(level=logging.DEBUG) |
There was a problem hiding this comment.
The logging level is set to DEBUG globally, which could expose sensitive information in production. Consider making the logging level configurable.
- logging.basicConfig(level=logging.DEBUG)
+ logging_level = os.getenv('LOGGING_LEVEL', 'WARNING').upper()
+ logging.basicConfig(level=getattr(logging, logging_level))Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| logging.basicConfig(level=logging.DEBUG) | |
| logging_level = os.getenv('LOGGING_LEVEL', 'WARNING').upper() | |
| logging.basicConfig(level=getattr(logging, logging_level)) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 21
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- pyproject.toml
Files selected for processing (20)
- .pylintrc (1 hunks)
- phlop/main.py (1 hunks)
- phlop/app/main.py (1 hunks)
- phlop/app/cmake.py (1 hunks)
- phlop/app/sleep.py (1 hunks)
- phlop/app/stats_man.py (1 hunks)
- phlop/dict.py (1 hunks)
- phlop/logger.py (1 hunks)
- phlop/os.py (2 hunks)
- phlop/proc.py (3 hunks)
- phlop/procs/init.py (1 hunks)
- phlop/procs/main.py (1 hunks)
- phlop/procs/runtimer.py (1 hunks)
- phlop/reflection.py (1 hunks)
- phlop/run/main.py (1 hunks)
- phlop/run/test_cases.py (1 hunks)
- phlop/testing/main.py (1 hunks)
- phlop/testing/parallel_processor.py (4 hunks)
- phlop/testing/test_cases.py (1 hunks)
- setup.py (1 hunks)
Files skipped from review due to trivial changes (6)
- phlop/main.py
- phlop/app/sleep.py
- phlop/logger.py
- phlop/procs/main.py
- phlop/run/main.py
- setup.py
Files skipped from review as they are similar to previous changes (5)
- phlop/app/main.py
- phlop/dict.py
- phlop/os.py
- phlop/reflection.py
- phlop/testing/main.py
Additional comments: 17
.pylintrc (1)
- 1-2: Please confirm that ignoring type checks for the
ValDictclass is intentional and that the class's design justifies this exclusion from type checking.phlop/app/stats_man.py (10)
23-28: The
ProcessCaptureInfodata class is a good addition for encapsulating process capture information.31-46: Refactoring argument parsing into the
cli_args_parserfunction improves modularity and readability.49-51: The
verify_cli_argsfunction is a good addition for separating the concern of argument verification.82-90: Using a multiprocessing
QueueinRuntimeStatsManagerfor inter-process communication is a good enhancement.73-79: Updating the
capture_nowfunction to collect additional process statistics is a valuable enhancement.130-143: Reorganizing the
mainfunction to use the new argument parsing and process management logic is a good practice.122-127: The addition of the
print_summaryfunction to display process statistics is a good enhancement for debugging and monitoring.69-71: The
bytes_as_mbutility function is a simple and correct implementation for converting bytes to megabytes.65-66: The
signal_handlerfunction is a standard implementation for handling signals in Python.54-62: The
check_pidfunction is a standard and correct implementation for checking the existence of a Unix PID.phlop/proc.py (2)
22-26: The
run_rawfunction is a new addition and seems to be implemented correctly. It provides more control over subprocesses by allowing the redirection of input/output streams based on thequietparameter.1-10: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [8-19]
Verify that the
runfunction has been updated to accept a list of strings as arguments and a newquietparameter, as these changes are not visible in the provided hunk.phlop/procs/__init__.py (1)
- 1-1: The hunk for
phlop/procs/__init__.pyis empty, indicating no changes were made. Please confirm if this is intentional or if there was an oversight in the commit.phlop/run/test_cases.py (3)
20-42: The
cli_args_parserfunction has been updated with new arguments and help descriptions. Ensure that the help descriptions are clear and match the intended functionality of the arguments.57-68: The
get_test_casesfunction has been refactored to include conditional logic based on thecmakeflag. Verify that the logic correctly handles the different scenarios and that the function returns the expected values.71-85: The
mainfunction has been refactored to utilize the new argument parsing and processing functions. Ensure that the new functions are being called correctly and that the overall flow of themainfunction is maintained.
| def config(path, cxx_flags=None, use_ninja=False, use_ccache=False, extra=""): | ||
| cmd = make_config_str(path, cxx_flags, use_ninja, extra) |
There was a problem hiding this comment.
The use_ccache parameter is not being passed to make_config_str from the config function. This should be corrected to ensure that the use_ccache parameter is used when generating the command string.
- cmd = make_config_str(path, cxx_flags, use_ninja, extra)
+ cmd = make_config_str(path, cxx_flags, use_ninja, use_ccache, extra)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def config(path, cxx_flags=None, use_ninja=False, use_ccache=False, extra=""): | |
| cmd = make_config_str(path, cxx_flags, use_ninja, extra) | |
| def config(path, cxx_flags=None, use_ninja=False, use_ccache=False, extra=""): | |
| cmd = make_config_str(path, cxx_flags, use_ninja, use_ccache, extra) |
| if __name__ == "__main__": | ||
| list_tests("build") |
There was a problem hiding this comment.
Consider allowing the build directory to be specified as a command-line argument to the script for greater flexibility.
| class GoogleTestCaseExtractor: | ||
| def __call__(self, ctest_test): | ||
| ... | ||
| # not configured, assumed fast per file | ||
| # print("GoogleTestCaseExtractor") | ||
| # exec binary with `--gtest_list_tests` and see if it doesn't fail | ||
| # p = run( | ||
| # ctest_test.cmd + " --gtest_list_tests --gtest_output=json:gtest.json", | ||
| # working_dir=ctest_test.working_dir, | ||
| # ) | ||
| # print(p.stdout) | ||
|
|
||
| return None |
There was a problem hiding this comment.
The GoogleTestCaseExtractor class contains commented-out code and returns None. If this is intentional and the class is not yet implemented, consider removing the commented-out code to avoid confusion, or provide an explanation for its presence.
| class PythonUnitTestCaseExtractor: | ||
| def __call__(self, ctest_test): | ||
| if "python3" in ctest_test.cmd: # hacky | ||
| return load_test_cases_from_cmake(ctest_test) | ||
| return None |
There was a problem hiding this comment.
The PythonUnitTestCaseExtractor still uses a simple string check to determine if a test is a Python unit test. Consider implementing a more robust method for identifying Python unit tests to avoid false positives or negatives.
| cmd = test_cmd_fn(type(suite), suite._testMethodName) | ||
| tests += [ | ||
| TestCase( | ||
| cmd=f"{test_cmd_pre} {cmd} {test_cmd_post}".strip(), |
There was a problem hiding this comment.
The command construction for test cases still uses string concatenation. Consider using a list and join to construct the command to avoid issues with extra spaces or improper formatting.
- cmd=f"{test_cmd_pre} {cmd} {test_cmd_post}".strip(),
+ cmd=" ".join(filter(None, [test_cmd_pre, cmd, test_cmd_post])),Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| cmd=f"{test_cmd_pre} {cmd} {test_cmd_post}".strip(), | |
| cmd=" ".join(filter(None, [test_cmd_pre, cmd, test_cmd_post])), |
phlop/procs/runtimer.py
Outdated
| start = time.time() | ||
|
|
||
| self.run_time = time.time() - start |
There was a problem hiding this comment.
The initial calculation of run_time is unnecessary and does not reflect the actual run time of the command. This line can be removed.
- self.run_time = time.time() - startCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| start = time.time() | |
| self.run_time = time.time() - start | |
| start = time.time() |
| def out(self, ignore_exit_code=False): | ||
| if not ignore_exit_code and self.exitcode > 0: | ||
| raise RuntimeError(f"phlop.RunTimer error: {self.stderr}") |
There was a problem hiding this comment.
The out method raises a generic RuntimeError. Consider creating a custom exception class for better error handling and clarity.
| env: dict = {}, # dict[str, str] # eventually | ||
| working_dir=None, | ||
| log_file_path=None, | ||
| **kwargs, |
There was a problem hiding this comment.
The kwargs and ekwargs are used to pass additional parameters to subprocess.run, but there is no validation or documentation on what can be passed. Consider adding validation or documentation for these parameters.
| if working_dir: | ||
| with pushd(working_dir): | ||
| run() | ||
| else: | ||
| run() |
There was a problem hiding this comment.
The pushd context manager is used to change the working directory, but there is no check to ensure that the directory exists or is accessible. Consider adding such a check to prevent runtime errors.
|
|
||
| from phlop.os import pushd, write_to_file | ||
| from phlop.procs.runtimer import RunTimer |
There was a problem hiding this comment.
The RunTimer class is imported but according to the summary, it has been removed. Please ensure that the run function is updated accordingly to not use a removed class.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 18
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- pyproject.toml
Files selected for processing (25)
- .pylintrc (1 hunks)
- phlop/init.py (1 hunks)
- phlop/main.py (1 hunks)
- phlop/app/init.py (1 hunks)
- phlop/app/main.py (1 hunks)
- phlop/app/cmake.py (1 hunks)
- phlop/app/git.py (2 hunks)
- phlop/app/perf.py (2 hunks)
- phlop/app/sleep.py (1 hunks)
- phlop/dict.py (1 hunks)
- phlop/logger.py (1 hunks)
- phlop/os.py (2 hunks)
- phlop/proc.py (3 hunks)
- phlop/procs/init.py (1 hunks)
- phlop/procs/main.py (1 hunks)
- phlop/procs/runtimer.py (1 hunks)
- phlop/reflection.py (1 hunks)
- phlop/run/main.py (1 hunks)
- phlop/run/stats_man.py (1 hunks)
- phlop/run/test_cases.py (1 hunks)
- phlop/string.py (1 hunks)
- phlop/testing/main.py (1 hunks)
- phlop/testing/parallel_processor.py (4 hunks)
- phlop/testing/test_cases.py (1 hunks)
- setup.py (1 hunks)
Files skipped from review due to trivial changes (7)
- .pylintrc
- phlop/init.py
- phlop/main.py
- phlop/app/main.py
- phlop/app/sleep.py
- phlop/procs/init.py
- setup.py
Files skipped from review as they are similar to previous changes (8)
- phlop/dict.py
- phlop/logger.py
- phlop/os.py
- phlop/proc.py
- phlop/procs/main.py
- phlop/reflection.py
- phlop/run/main.py
- phlop/testing/main.py
Additional comments: 19
phlop/app/git.py (2)
36-39: The
checkoutfunction has been correctly updated to remove therecreateparameter, and the logic has been adjusted to only consider thecreateparameter.33-33: The previous suggestion to use
git rev-parse --verifyinstead ofgit show-branchfor checking branch existence has not been implemented.phlop/app/perf.py (3)
1-6: The addition of empty lines at the beginning of the file seems to be a formatting change and does not affect the functionality.
16-16: Verify that the
runfunction returns an object that has anoutmethod and that this method provides the expected output format for further processing.19-19: Ensure that the output of the
perf -vcommand is consistent with the parsing logic, specifically that the version number is the last element in the output after splitting by space and then by period.phlop/procs/runtimer.py (2)
74-78: Using the
pushdcontext manager to change the working directory when provided is a good practice.70-72: Logging the output to files when
log_file_pathis provided is a good practice.phlop/run/stats_man.py (1)
- 38-53: The
cli_args_parserfunction defines command-line arguments. Ensure that the new arguments introduced are consistent with the PR's objectives and that they are documented and tested.phlop/run/test_cases.py (1)
- 22-22: The function
parse_cli_argshas been renamed tocli_args_parser. Ensure that all references to this function are updated throughout the codebase to reflect the new name.phlop/testing/parallel_processor.py (4)
19-37: The refactoring of the
CallableTestclass to accept atest_caseobject instead of a command string aligns with the PR's objectives to improve CMake and CTest integration.58-62: The early return in the
processfunction when there are no tests to run is a good performance optimization.85-91: The simplification of the
argsparameter in theProcessconstructor improves code readability.107-112: The update in the
waiterfunction to print test case information using thetest_caseattribute is consistent with the refactoring of theCallableTestclass.phlop/testing/test_cases.py (6)
49-61: The previous comment about the
GoogleTestCaseExtractorclass still applies. The class contains commented-out code and returnsNone, indicating it is not yet implemented or is a placeholder.64-68: The previous comment about the
PythonUnitTestCaseExtractorclass still applies. The class uses a simple string check to determine if a test is a Python unit test, which could lead to false positives or negatives.93-93: The previous comment about the command construction for test cases still applies. Consider using a list and
jointo construct the command to avoid issues with extra spaces or improper formatting.104-105: The previous comment about the
load_test_cases_from_cmakefunction still applies. Ensure that changes toPYTHONPATHare scoped and do not affect other parts of the system or subsequent tests.134-138: The previous comment about handling the case where no extractors return a result, which could lead to an empty
TestBatch, still applies. Consider adding a check after the loop and handling the scenario wheretest_casesremains empty.41-44: The previous comment about potential issues with path operations in the
DefaultTestCaseExtractorclass still applies. Ensure thatctest_test.working_diris always a subdirectory of_LOG_DIRto avoidValueErrorfromPath.relative_to().
| def config(path, cxx_flags=None, use_ninja=False, use_ccache=False, extra=""): | ||
| cmd = make_config_str(path, cxx_flags, use_ninja, extra) | ||
| run(cmd, capture_output=False) |
There was a problem hiding this comment.
The use_ccache parameter is not being passed to make_config_str from the config function. This should be corrected to ensure that the use_ccache parameter is used when generating the command string.
- cmd = make_config_str(path, cxx_flags, use_ninja, extra)
+ cmd = make_config_str(path, cxx_flags, use_ninja, use_ccache, extra)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def config(path, cxx_flags=None, use_ninja=False, use_ccache=False, extra=""): | |
| cmd = make_config_str(path, cxx_flags, use_ninja, extra) | |
| run(cmd, capture_output=False) | |
| def config(path, cxx_flags=None, use_ninja=False, use_ccache=False, extra=""): | |
| cmd = make_config_str(path, cxx_flags, use_ninja, use_ccache, extra) | |
| run(cmd, capture_output=False) |
| properties: list = field(default_factory=lambda: [{}]) # list[dict] # eventually | ||
|
|
||
| env: dict = field(default_factory=lambda: {}) # dict[str, str] # eventually | ||
| working_dir: str = field(default_factory=lambda: None) |
There was a problem hiding this comment.
The working_dir field still has a default factory that returns None, which may lead to type inconsistencies. Consider changing the default to an empty string or handling None values appropriately where working_dir is used.
| def list_tests(build_dir=None): | ||
| cmd = "".join( | ||
| [ | ||
| s | ||
| for s in [ | ||
| "ctest ", | ||
| f"--test-dir {build_dir} " if build_dir else "", | ||
| "--show-only=json-v1", | ||
| ] | ||
| if s | ||
| ] | ||
| ) |
There was a problem hiding this comment.
The command string in list_tests can be constructed more cleanly using a list comprehension and the join method.
- cmd = "".join(
- [
- s
- for s in [
- "ctest ",
- f"--test-dir {build_dir} " if build_dir else "",
- "--show-only=json-v1",
- ]
- if s
- ]
- )
+ cmd = " ".join(filter(None, ["ctest", f"--test-dir {build_dir}" if build_dir else "", "--show-only=json-v1"]))Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def list_tests(build_dir=None): | |
| cmd = "".join( | |
| [ | |
| s | |
| for s in [ | |
| "ctest ", | |
| f"--test-dir {build_dir} " if build_dir else "", | |
| "--show-only=json-v1", | |
| ] | |
| if s | |
| ] | |
| ) | |
| def list_tests(build_dir=None): | |
| cmd = " ".join(filter(None, ["ctest", f"--test-dir {build_dir}" if build_dir else "", "--show-only=json-v1"])) |
| def version(): | ||
| pass | ||
|
|
There was a problem hiding this comment.
The version function is currently a placeholder with no implementation. Please ensure that the implementation is complete or provide a TODO comment if it's intended to be implemented later.
| @@ -63,13 +52,14 @@ def runner(runnable, queue): | |||
| def print_tests(batches): | |||
| for batch in batches: | |||
| for test in batch.tests: | |||
| print(test) | |||
| print(test.cmd) | |||
There was a problem hiding this comment.
The print_tests function should be updated to access the cmd attribute from the test_case attribute of the test object, as the direct cmd attribute on test no longer exists.
- print(test.cmd)
+ print(test.test_case.cmd)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| print(test.cmd) | |
| print(test.test_case.cmd) |
phlop/run/stats_man.py
Outdated
| def verify_cli_args(cli_args): | ||
| cli_args.interval = int(cli_args.interval) | ||
| return cli_args |
There was a problem hiding this comment.
The verify_cli_args function converts the interval argument to an integer without validation. Consider adding error handling for non-integer inputs to prevent runtime exceptions.
def verify_cli_args(cli_args):
+ try:
cli_args.interval = int(cli_args.interval)
+ except ValueError:
+ raise ValueError("Interval must be an integer")
return cli_argsCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def verify_cli_args(cli_args): | |
| cli_args.interval = int(cli_args.interval) | |
| return cli_args | |
| def verify_cli_args(cli_args): | |
| try: | |
| cli_args.interval = int(cli_args.interval) | |
| except ValueError: | |
| raise ValueError("Interval must be an integer") | |
| return cli_args |
| class RuntimeStatsManager: | ||
| def __init__(self, pid, interval=_default_interval): | ||
| self.pid = pid | ||
| self.interval = interval | ||
| self.pqueue = Queue() | ||
| self.data = {} | ||
| self.p = Process(target=RuntimeStatsManager._run, args=(self,)) | ||
| self.p.start() | ||
| self.data = self.pqueue.get() |
There was a problem hiding this comment.
The RuntimeStatsManager class starts a process in its constructor. Starting a process during object initialization can be risky and might lead to resources not being released properly if an exception occurs. Consider using a factory method or a context manager to handle process lifecycle.
phlop/run/stats_man.py
Outdated
| @staticmethod | ||
| def _run(self): | ||
| data = ProcessCaptureInfo() | ||
| signal.signal(signal.SIGINT, signal_handler) | ||
| while check_pid(self.pid): | ||
| try: | ||
| capture_now(self.pid, data) | ||
| except psutil.AccessDenied: | ||
| break | ||
| time.sleep(self.interval) | ||
| self.pqueue.put(data) |
There was a problem hiding this comment.
The _run method is a static method but is called with self as an argument. This is unconventional and could lead to confusion. Consider refactoring to use instance methods or class methods with appropriate arguments.
- @staticmethod
- def _run(self):
+ def _run(self):
data = ProcessCaptureInfo()
signal.signal(signal.SIGINT, signal_handler)
while check_pid(self.pid):
try:
capture_now(self.pid, data)
except psutil.AccessDenied:
break
time.sleep(self.interval)
self.pqueue.put(data)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @staticmethod | |
| def _run(self): | |
| data = ProcessCaptureInfo() | |
| signal.signal(signal.SIGINT, signal_handler) | |
| while check_pid(self.pid): | |
| try: | |
| capture_now(self.pid, data) | |
| except psutil.AccessDenied: | |
| break | |
| time.sleep(self.interval) | |
| self.pqueue.put(data) | |
| def _run(self): | |
| data = ProcessCaptureInfo() | |
| signal.signal(signal.SIGINT, signal_handler) | |
| while check_pid(self.pid): | |
| try: | |
| capture_now(self.pid, data) | |
| except psutil.AccessDenied: | |
| break | |
| time.sleep(self.interval) | |
| self.pqueue.put(data) |
|
|
||
|
|
||
| def verify_cli_args(cli_args): | ||
| if cli_args.cores == "a" or cli_args.cores == "all": | ||
| cli_args.cores = cpu_count() | ||
| cli_args.cores = multiprocessing.cpu_count() | ||
| cli_args.cores = int(cli_args.cores) |
There was a problem hiding this comment.
The conversion of cli_args.cores to an integer is redundant since the --cores argument is already defined as an integer type in the argument parser. This line should be removed.
- cli_args.cores = int(cli_args.cores)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| cli_args.cores = int(cli_args.cores) |
| except: | ||
| e = sys.exc_info()[0] | ||
| print(f"Error: Unknown Error {e}") |
There was a problem hiding this comment.
The catch-all exception handler is still present, which can obscure the actual error. Consider catching specific exceptions to provide more detailed error information and handling.
- except:
- e = sys.exc_info()[0]
- print(f"Error: Unknown Error {e}")
+ except Exception as e:
+ print(f"Error: {e.__class__.__name__}: {e}")Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| except: | |
| e = sys.exc_info()[0] | |
| print(f"Error: Unknown Error {e}") | |
| except Exception as e: | |
| print(f"Error: {e.__class__.__name__}: {e}") |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 12
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- pyproject.toml
Files selected for processing (26)
- .pylintrc (1 hunks)
- phlop/init.py (1 hunks)
- phlop/main.py (1 hunks)
- phlop/app/init.py (1 hunks)
- phlop/app/main.py (1 hunks)
- phlop/app/cmake.py (1 hunks)
- phlop/app/git.py (2 hunks)
- phlop/app/perf.py (2 hunks)
- phlop/app/sleep.py (1 hunks)
- phlop/dict.py (1 hunks)
- phlop/logger.py (1 hunks)
- phlop/os.py (2 hunks)
- phlop/proc.py (3 hunks)
- phlop/procs/init.py (1 hunks)
- phlop/procs/main.py (1 hunks)
- phlop/procs/runtimer.py (1 hunks)
- phlop/reflection.py (1 hunks)
- phlop/run/main.py (1 hunks)
- phlop/run/stats_man.py (1 hunks)
- phlop/run/test_cases.py (1 hunks)
- phlop/string.py (1 hunks)
- phlop/sys.py (1 hunks)
- phlop/testing/main.py (1 hunks)
- phlop/testing/parallel_processor.py (5 hunks)
- phlop/testing/test_cases.py (1 hunks)
- setup.py (1 hunks)
Files skipped from review due to trivial changes (9)
- .pylintrc
- phlop/init.py
- phlop/main.py
- phlop/app/sleep.py
- phlop/logger.py
- phlop/procs/init.py
- phlop/procs/main.py
- phlop/run/main.py
- setup.py
Files skipped from review as they are similar to previous changes (11)
- phlop/app/main.py
- phlop/app/cmake.py
- phlop/app/git.py
- phlop/app/perf.py
- phlop/dict.py
- phlop/os.py
- phlop/procs/runtimer.py
- phlop/reflection.py
- phlop/string.py
- phlop/testing/main.py
- phlop/testing/parallel_processor.py
Additional comments: 7
phlop/proc.py (2)
- 10-10: The
RunTimerclass has been imported fromphlop.procs.runtimerbut the usage in therunfunction needs to be verified to ensure it aligns with the new implementation.Verification successful
The
RunTimerclass has been successfully located in thephlop/procs/runtimer.pyfile, and its usage inphlop/proc.pyhas been confirmed. The initial concerns have been addressed, and the changes appear to be correctly implemented.* 28-32: The `run_raw` function has been added to run subprocesses with more control over input/output streams. This is a significant addition and appears to be implemented correctly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new RunTimer class is compatible with its usage in the run function. ast-grep --lang python --pattern $'class RunTimer { $$$ __init__($_, $_, $_, $_, $$$) { $$$ } }'Length of output: 452
Script:
#!/bin/bash # Verify the existence and constructor signature of the RunTimer class in its new location. ast-grep --lang python --pattern $'class RunTimer { $$$ __init__($$$) { $$$ } }'Length of output: 436
Script:
#!/bin/bash # Verify the usage of RunTimer in the run function within phlop/proc.py. ast-grep --lang python --pattern $'def run($$$) { $$$ RunTimer($$$) $$$ }'Length of output: 426
Script:
#!/bin/bash # Search for the RunTimer class in the phlop/procs/runtimer.py file. rg 'class RunTimer' -- phlop/procs/runtimer.pyLength of output: 64
Script:
#!/bin/bash # Search for the usage of RunTimer in the phlop/proc.py file. rg 'RunTimer' -- phlop/proc.pyLength of output: 93
phlop/run/test_cases.py (4)
22-45:
Thecli_args_parserfunction has been updated with new argument options and default values, which aligns with the PR objectives to enhance CMake and CTest support.51-51:
The previous comment regarding the redundant conversion ofcli_args.coresto an integer still stands, as the--coresargument is already defined as an integer type in the argument parser.59-70:
Theget_test_casesfunction has been correctly updated to include conditional logic for handling CMake tests based on thecmakeflag.73-87:
The refactoring of themainfunction to use the new argument parsing and processing functions is consistent with the PR objectives. The previous comment regarding the catch-all exception handler still applies.phlop/testing/test_cases.py (1)
- 26-26: Ensure that the
log_file_pathis being used correctly and safely throughout the code, especially in the context of file operations and potential security implications.
| # | ||
| # | ||
| # | ||
| # | ||
| # |
There was a problem hiding this comment.
The hunk indicates that the entire content of phlop/app/__init__.py has been removed, leaving only comments or whitespace. This is a substantial change that could potentially break any part of the codebase that depends on the removed functions. It is crucial to ensure that all references to the removed functions are either updated or removed, and that any necessary functionality is reimplemented elsewhere if needed.
| if not Path(cli_args.dir).exists(): | ||
| raise RuntimeError( | ||
| "phlop.run.test_cases error: directory provided does not exist" | ||
| ) |
There was a problem hiding this comment.
Consider enhancing the error message by including the directory path that does not exist.
- "phlop.run.test_cases error: directory provided does not exist"
+ f"phlop.run.test_cases error: directory '{cli_args.dir}' provided does not exist"Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if not Path(cli_args.dir).exists(): | |
| raise RuntimeError( | |
| "phlop.run.test_cases error: directory provided does not exist" | |
| ) | |
| if not Path(cli_args.dir).exists(): | |
| raise RuntimeError( | |
| f"phlop.run.test_cases error: directory '{cli_args.dir}' provided does not exist" | |
| ) |
| @contextmanager | ||
| def extend_sys_path(paths): | ||
| if isinstance(paths, str): | ||
| paths = [paths] | ||
| old_path = sys.path[:] | ||
| sys.path.extend(paths) | ||
| try: | ||
| yield | ||
| finally: | ||
| sys.path = old_path |
There was a problem hiding this comment.
Consider adding a docstring to the extend_sys_path function to explain its purpose, usage, and the parameters it accepts. This will improve code readability and maintainability.
@contextmanager
+ def extend_sys_path(paths):
+ """
+ Temporarily add directories to the system path.
+
+ This context manager extends the system path with the given paths
+ for the duration of the context. The original system path is restored
+ afterwards.
+
+ Args:
+ paths (str or list): A path or list of paths to add to the system path.
+ """
if isinstance(paths, str):
paths = [paths]
old_path = sys.path[:]
sys.path.extend(paths)
try:
yield
finally:
sys.path = old_pathCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| @contextmanager | |
| def extend_sys_path(paths): | |
| if isinstance(paths, str): | |
| paths = [paths] | |
| old_path = sys.path[:] | |
| sys.path.extend(paths) | |
| try: | |
| yield | |
| finally: | |
| sys.path = old_path | |
| @contextmanager | |
| def extend_sys_path(paths): | |
| """ | |
| Temporarily add directories to the system path. | |
| This context manager extends the system path with the given paths | |
| for the duration of the context. The original system path is restored | |
| afterwards. | |
| Args: | |
| paths (str or list): A path or list of paths to add to the system path. | |
| """ | |
| if isinstance(paths, str): | |
| paths = [paths] | |
| old_path = sys.path[:] | |
| sys.path.extend(paths) | |
| try: | |
| yield | |
| finally: | |
| sys.path = old_path |
| class TestBatch: | ||
| def __init__(self, tests, cores=1): | ||
| self.tests = tests | ||
| self.cores = cores | ||
|
|
There was a problem hiding this comment.
The TestBatch class currently does not have any methods to process the tests or manage concurrency based on the cores attribute. Please clarify if this is an incomplete implementation or if additional code is planned to utilize these attributes.
| logging.basicConfig(level=logging.DEBUG) | ||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Consider making the logging level configurable or setting a less verbose default level for production use.
- logging.basicConfig(level=logging.DEBUG)
+ logging.basicConfig(level=logging.INFO) # Or make this configurableCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| logging.basicConfig(level=logging.DEBUG) | |
| logger = logging.getLogger(__name__) | |
| logging.basicConfig(level=logging.INFO) # Or make this configurable | |
| logger = logging.getLogger(__name__) |
| def __del__(self): | ||
| if check_pid(self.p.pid): | ||
| os.kill(self.p.pid, signal.SIGINT) | ||
| self.join() |
There was a problem hiding this comment.
Using the destructor method __del__ to terminate the process is unreliable. Consider using a context manager or explicit cleanup method.
| def join(self): | ||
| if not self.pid: | ||
| return | ||
| while self.p.exitcode is None and check_pid(self.p.pid): | ||
| time.sleep(1) | ||
| self.pid = 0 | ||
| return self |
There was a problem hiding this comment.
The join method sets self.pid to 0 after waiting for the process to exit. Consider making the method idempotent or documenting this behavior.
| def print_summary(statsman): | ||
| print("summary:") | ||
| print("\tCPU avg:", np.average(statsman.data.cpu_load)) | ||
| print("\tCPU max:", np.max(statsman.data.cpu_load)) | ||
|
|
||
| print("\tFDS avg:", np.average(statsman.data.fds)) | ||
| print("\tFDS max:", np.max(statsman.data.fds)) | ||
|
|
||
| print("\tMEM avg:", np.average(statsman.data.mem_usage)) | ||
| print("\tMEM max:", np.max(statsman.data.mem_usage)) |
There was a problem hiding this comment.
Ensure that the statsman.data lists are not empty before calling numpy functions to avoid ValueError.
def print_summary(statsman):
+ if statsman.data.cpu_load:
print("CPU avg:", np.average(statsman.data.cpu_load))
print("CPU max:", np.max(statsman.data.cpu_load))
# Repeat checks for fds and mem_usageCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def print_summary(statsman): | |
| print("summary:") | |
| print("\tCPU avg:", np.average(statsman.data.cpu_load)) | |
| print("\tCPU max:", np.max(statsman.data.cpu_load)) | |
| print("\tFDS avg:", np.average(statsman.data.fds)) | |
| print("\tFDS max:", np.max(statsman.data.fds)) | |
| print("\tMEM avg:", np.average(statsman.data.mem_usage)) | |
| print("\tMEM max:", np.max(statsman.data.mem_usage)) | |
| def print_summary(statsman): | |
| print("summary:") | |
| if statsman.data.cpu_load: | |
| print("\tCPU avg:", np.average(statsman.data.cpu_load)) | |
| print("\tCPU max:", np.max(statsman.data.cpu_load)) | |
| if statsman.data.fds: | |
| print("\tFDS avg:", np.average(statsman.data.fds)) | |
| print("\tFDS max:", np.max(statsman.data.fds)) | |
| if statsman.data.mem_usage: | |
| print("\tMEM avg:", np.average(statsman.data.mem_usage)) | |
| print("\tMEM max:", np.max(statsman.data.mem_usage)) |
| def main(): | ||
| parser = cli_args_parser() | ||
| cli_args = verify_cli_args(parser.parse_args()) | ||
| try: | ||
| proc = run_raw(cli_args.remaining, quiet=cli_args.quiet) | ||
| statsman = RuntimeStatsManager(proc.pid, cli_args.interval).join() | ||
| print_summary(statsman) | ||
| except (Exception, SystemExit) as e: | ||
| logger.exception(e) | ||
| parser.print_help() | ||
| except: | ||
| e = sys.exc_info()[0] | ||
| print(f"Error: Unknown Error {e}") | ||
|
|
There was a problem hiding this comment.
The main function catches a generic exception and prints an unknown error message. Consider logging the full exception details.
- print(f"Error: Unknown Error {e}")
+ logger.exception("An unexpected error occurred", exc_info=e)Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def main(): | |
| parser = cli_args_parser() | |
| cli_args = verify_cli_args(parser.parse_args()) | |
| try: | |
| proc = run_raw(cli_args.remaining, quiet=cli_args.quiet) | |
| statsman = RuntimeStatsManager(proc.pid, cli_args.interval).join() | |
| print_summary(statsman) | |
| except (Exception, SystemExit) as e: | |
| logger.exception(e) | |
| parser.print_help() | |
| except: | |
| e = sys.exc_info()[0] | |
| print(f"Error: Unknown Error {e}") | |
| def main(): | |
| parser = cli_args_parser() | |
| cli_args = verify_cli_args(parser.parse_args()) | |
| try: | |
| proc = run_raw(cli_args.remaining, quiet=cli_args.quiet) | |
| statsman = RuntimeStatsManager(proc.pid, cli_args.interval).join() | |
| print_summary(statsman) | |
| except (Exception, SystemExit) as e: | |
| logger.exception(e) | |
| parser.print_help() | |
| except: | |
| e = sys.exc_info()[0] | |
| logger.exception("An unexpected error occurred", exc_info=e) |
| raise ValueError("do better") | ||
| return run(f"which {bin}").returncode == 0 | ||
| return run(f"which {bin}").exitcode == 0 |
There was a problem hiding this comment.
The binary_exists_on_path function still raises a ValueError with a non-descriptive message. This should be replaced with a proper check or a more informative error message.
- raise ValueError("do better")
+ # Implement the actual check for binary existence or provide a descriptive error messageCommittable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| raise ValueError("do better") | |
| return run(f"which {bin}").returncode == 0 | |
| return run(f"which {bin}").exitcode == 0 | |
| # Implement the actual check for binary existence or provide a descriptive error message | |
| return run(f"which {bin}").exitcode == 0 |
Summary by CodeRabbit
New Features
ValDictclass extending the functionality of the built-indict.Refactor
phlop.apprelated to system operations and process management.RunTimerclass with an updated version.runtime statistics managementandtest casefunctionalities with new classes and methods.Bug Fixes
checkoutfunction inphlop.app/git.pyby removing an unnecessary parameter.versionfunction inphlop.app/perf.py.pushdfunction and added awrite_to_filefunction inphlop/os.py.decode_bytesfunction inphlop/string.py.Documentation
Style
.pylintrcconfiguration.Chores