Changes for hlktrace service intergation#787
Conversation
|
Sorry. for the noise, for some reason GitHub showed single commit to me. |
e5d8c1b to
170ca62
Compare
170ca62 to
642e44f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the test execution flow by adding support for pre- and post-test commands, including file operations and variable replacements. The code is well-structured, with new logic encapsulated in dedicated methods and models. I've provided a few suggestions to improve consistency in command handling, clarify some logic, and update the new documentation to be more comprehensive. Overall, these are solid changes that increase the flexibility of test configuration.
| def run_host_test_command(command) | ||
| return unless command.host_run | ||
|
|
||
| @logger.info("Running command (#{command.desc}) on host") | ||
| run_cmd(command.host_run) | ||
| end |
There was a problem hiding this comment.
For consistency with run_guest_test_command, this method should also support variable replacements. This would allow host_run commands to use dynamic values like @workspace@ or @safe_test_name@. You'll also need to update the call site in run_test_commands to pass the replacement map.
def run_host_test_command(command, replacement)
return unless command.host_run
@logger.info("Running command (#{command.desc}) on host")
updated_command = replacement.replace(command.host_run)
@logger.debug("Running command after replacement (#{command.desc}) on host: #{updated_command}")
run_cmd(updated_command)
endThere was a problem hiding this comment.
I agree with consistency, but currently this is unclear what we should provide to host_run. Let's leave as is until we will have real understanding what is useful for host_run.
@YanVugenfirer What do you think?
There was a problem hiding this comment.
Let's leave for now as is.
There was a problem hiding this comment.
Pull Request Overview
This pull request integrates HLK log service functionality into AutoHCK by implementing a replacement map system for dynamic variable substitution in commands and file paths, along with file action capabilities for managing files between host and client VMs during test execution.
- Adds
ReplacementMapfunctionality to clients and test execution for dynamic path/variable substitution - Implements file action system for copying/moving files between host and client VMs with configurable direction
- Updates rtoolsHCK dependency from v0.6.0 to v0.6.1 and regenerates type signatures
Reviewed Changes
Copilot reviewed 9 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/setupmanagers/hckclient.rb | Adds replacement_map attribute and integrates it into post-start commands and driver installation |
| lib/models/command_info.rb | Defines FileActionConfig and FileActionDirection for file operations configuration |
| lib/engines/hcktest/tests.rb | Implements file action execution logic and integrates replacement map into test command execution |
| lib/auxiliary/replacement_map.rb | Adds dump_string method for debugging replacement maps |
| docs/Test-config-behavior.md | Documents test configuration options, execution order, and file action behavior |
| Gemfile | Updates rtoolsHCK reference from v0.6.0 to v0.6.1 |
| Gemfile.lock | Updates dependencies including rtoolsHCK, date, erb, psych, rdoc, rexml, stringio, and adds tsort |
| sorbet/rbi/gems/*.rbi | Autogenerated type signature files for updated and new gem dependencies |
Files not reviewed (2)
- sorbet/rbi/gems/erb@6.0.0.rbi: Language not supported
- sorbet/rbi/gems/tsort@0.2.0.rbi: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…commands Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
…mands Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
642e44f to
0fb7d6a
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the test configuration and execution flow, particularly around pre/post-test commands and file actions. It also includes necessary dependency updates. The new FileActionConfig and FileActionDirection models provide a structured way to handle file operations, which is a great improvement. The documentation for test configuration behavior is a valuable addition, although some default behaviors could be more explicitly stated. There are a few areas where code clarity and potential error handling could be improved.
| if files_action.remote_path.nil? && files_action.local_path.nil? | ||
| @logger.warn('Both remote and local paths are nil, skipping file action') | ||
| next | ||
| end |
There was a problem hiding this comment.
The check if files_action.remote_path.nil? && files_action.local_path.nil? correctly identifies a potentially misconfigured file action. While logging a warning is helpful, if both paths are nil, this might indicate a critical configuration error that should prevent the action from proceeding or raise an error, rather than just skipping it. This could prevent silent failures or unexpected behavior downstream.
if files_action.remote_path.nil? && files_action.local_path.nil?
raise EngineError, 'Both remote and local paths are nil for file action, cannot proceed.'
end| - If `remote_path` is empty, files will be copied to C:\ drive | ||
| - If `local_path` is empty, files will be copied to the current workspace |
There was a problem hiding this comment.
The default behaviors for remote_path and local_path when empty are currently described under "File Action Behavior". For better discoverability, it would be beneficial to explicitly state these defaults directly within the "File Action Configuration Options" section as well. This ensures users can see all configuration options and their default behaviors in one place.
NetKVM.json example:
{ "name": "Red Hat VirtIO Ethernet Adapter", "device": "virtio-net-pci", "inf": "netkvm.inf", "install_method": "PNP", "type": 0, "support": true, "extra_software": [ "hcktrace" ], "post_start_commands": [ { "desc": "Configure HCKTrace service for NETKVM", "guest_run": "Start-Process -Wait -FilePath hcktrace.exe -ArgumentList driver,netkvm,@NetKVM.driver_dir@,4" } ], "tests_config": [ { "tests": [ ".*" ], "pre_test_commands": [ { "desc": "Rename NetKVM ethernet adapter to SupportDevice0", "guest_run": "Rename-NetAdapter -Name (Get-NetAdapter -InterfaceDescription 'Red Hat VirtIO Ethernet Adapter').Name -NewName 'SupportDevice0'" }, { "desc": "Start HCKTrace recording", "guest_run": "Start-Process -Wait -FilePath hcktrace.exe -ArgumentList test,@safe_test_name@" } ], "post_test_commands": [ { "desc": "Stop HCKTrace recording", "guest_run": "Start-Process -Wait -FilePath hcktrace.exe -ArgumentList test,end", "files_action": [ { "remote_path": "C:/hcktrace/zip", "local_path": "@workspace@/@safe_test_name@-trace-@client_name@", "direction": "remote-to-local", "move": true, "allow_missing": true } ] } ] } ], "reject_test_names": [ "NDISTest 6.5 - [2 Machine] - MPE_Ethernet.xml", "NDISTest 6.0 - [1 Machine] - 1c_Mini6RSSOids", "PrivateCloudSimulator - Device.Network.LAN.10GbOrGreater", "Run RSC Tests", "DF - Embedded Signature Verification Test (Certification)", "DF - Embedded Signature Verification Test (Tuning and Validation)", "Hardware-enforced Stack Protection Compatibility Test" ] }