impl: reworked implementation #49
Conversation
- Simplified usage.
- Added `hang` and `return_code` to `LogContainer`.
- Added parametrized timeouts.
- Implemented automatic paths detection based on `cargo metadata`.
- Expected binary name is parametrized, with `rust_test_scenario` as
default value.
- Added type hints.
- Small README fixes.
- More flexible `add_log` method.
| p = Popen(command, stdout=PIPE, stdin=PIPE, stderr=PIPE, text=True) | ||
| try: | ||
| outs, errs = p.communicate(test_config_json, EXECUTE_TIMEOUT_S) | ||
| stdout, _ = p.communicate(test_config_str, execution_timeout) |
There was a problem hiding this comment.
We need to see stderr data in case of some error, e.g. panics are written there
| messages.sort(key=lambda m: m["timestamp"]) | ||
|
|
||
| return messages | ||
| # Convert to list of ResultEntry. |
There was a problem hiding this comment.
Leave only comments for not obvious steps. I would go for more functions, less comments.
|
|
||
| def execute( |
There was a problem hiding this comment.
Fragment to more, shorter functions
|
|
||
|
|
||
| def build_test_scenarios( | ||
| rust_scenarios_bin_name: str | None = None, |
There was a problem hiding this comment.
Set default values in the arguments directly, no need for None handling
| True if execution hanged. | ||
| """ | ||
| self._logs = entries or [] | ||
| self._return_code = return_code |
There was a problem hiding this comment.
Not sure if it is a good place to store other data than logs. This class often goes thru transformation LogContainer -> List[ResultEntry] -> LogContainer and this data will be lost. IMO return code should be delivered in a separate fixture if test wants to explicitly check that. Hang indicator has no value to the test body itself as it will fail in the tt.unfiltered_test_results.
| hang : bool | None | ||
| True if execution hanged. | ||
| """ | ||
| self._logs = entries or [] |
There was a problem hiding this comment.
Its not pythonic - better define an empty list as default in arguments
| def __init__(self): | ||
| self.logs = [] | ||
| def __init__( | ||
| self, entries: list[ResultEntry] | None = None, return_code: int | None = None, hang: bool | None = None |
There was a problem hiding this comment.
Instead of all those | None = None you can
from typing import Optional
return_code: Optional[int]
|
|
||
| def group_by(self, attribute: str) -> Dict[str, "LogContainer"]: | ||
| def group_by(self, attribute: str) -> dict[str, "LogContainer"]: |
There was a problem hiding this comment.
this is incorrect, it should be Dict from typing for type annotations
| raise FileNotFoundError( | ||
| f"Cargo.toml not found at {path_to_manifest}. Please provide a valid path to the rust test scenarios." | ||
| # Set default timeout. | ||
| metadata_timeout = cargo_metadata_timeout_s or 10.0 |
There was a problem hiding this comment.
define default directly in the function definition
hangandreturn_codetoLogContainer.cargo metadata.rust_test_scenarioas default value.add_logmethod.Resolve #48