-
Notifications
You must be signed in to change notification settings - Fork 18
Implement extensions mechanism and add HCKTrace #843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Summary of ChangesHello @kostyanf14, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust extension mechanism, significantly enhancing the modularity and extensibility of the test framework. It enables the dynamic loading of additional functionalities, exemplified by the new HCKTrace integration for automated driver tracing. This change streamlines the process of adding custom behaviors and tools to the testing pipeline without modifying core logic, improving overall flexibility and diagnostic capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements an extensions mechanism for the test framework and adds HCKTrace as the first extension. Extensions allow modular functionality to be added to tests without modifying driver configurations.
Changes:
- Added Extension model with support for extra software, post-start commands, and test configurations
- Integrated extensions throughout the test execution pipeline alongside drivers
- Created HCKTrace extension for driver tracing during test execution
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/models/extension.rb | New Extension model defining structure for modular test extensions |
| lib/models.rb | Registered Extension model in autoload list |
| lib/models/driver.rb | Changed extra_software from nilable to array with default for consistency |
| lib/engines/hcktest/hcktest.rb | Added extension loading, integrated extensions into post-start commands and test configurations |
| lib/engines/hcktest/tests.rb | Included extensions in test configuration selection logic |
| lib/setupmanagers/hckclient.rb | Added extension support in client setup and extracted driver replacement logic into new method |
| lib/engines/hcktest/extensions/hcktrace.json | HCKTrace extension configuration for driver tracing |
| lib/cli.rb | Added --extensions CLI option to specify extensions list |
| docs/Test-config-behavior.md | Documented new replacement variables for single-driver scenarios |
| lib/engines/hckinstall/hckinstall.rb | Refactored kit installer preparation into separate method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| end | ||
|
|
||
| def insert_driver_replacement(driver, one_driver, replacement) | ||
| client_replacement = replacement.transform_keys { |k| k.dup.insert(1, "#{driver.short}.") } |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code assumes driver.short is non-nil, but the Driver model allows short to be T.nilable(String). If driver.short is nil, this will raise a NoMethodError when attempting string interpolation. Add a nil check or ensure driver.short is always present before calling this method.
| "post_start_commands": [ | ||
| { | ||
| "desc": "Configure HCKTrace service for driver tracing", | ||
| "guest_run": "Start-Process -Wait -FilePath hcktrace.exe -ArgumentList driver,@driver_short_name@,@driver_dir@,4" |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument '4' at the end of the ArgumentList is unclear. Consider adding a comment in the JSON file or updating the desc field to explain what this numeric parameter represents (e.g., log level, trace verbosity).
| &method(:fs_test_image_format=)) | ||
|
|
||
| parser.on('--extensions <extensions_list>', Array, | ||
| 'List of extensions for run test', |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar issue: 'for run test' should be 'to run tests' or 'for running tests'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor VS Copilot haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable extensions mechanism, enhancing modularity. However, I've identified a critical bug in the prepare_extra_sw method that would prevent extra software from being installed correctly. Additionally, there's a high-severity issue in how driver-specific replacements are handled in multi-driver scenarios, which could lead to incorrect behavior. I've provided code suggestions to address both of these issues. The rest of the changes appear to correctly implement the new functionality.
| def insert_driver_replacement(driver, one_driver, replacement) | ||
| client_replacement = replacement.transform_keys { |k| k.dup.insert(1, "#{driver.short}.") } | ||
| @replacement_map.merge!(client_replacement) | ||
| @replacement_map.merge!({ '@driver_short_name@' => driver.short }) | ||
|
|
||
| # If there is only one driver, we can use generic keys without driver short name. | ||
| @replacement_map.merge!(replacement) if one_driver | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @driver_short_name@ replacement is being set unconditionally for every driver. In a multi-driver scenario, this will result in @driver_short_name@ holding the value of the last driver in the list, which can lead to incorrect behavior. According to the documentation, this replacement should only be available when a single driver is being tested.
To fix this, the logic for setting @driver_short_name@ should be moved inside the if one_driver block, along with the other generic driver replacements.
def insert_driver_replacement(driver, one_driver, replacement)
client_replacement = replacement.transform_keys { |k| k.dup.insert(1, "#{driver.short}.") }
@replacement_map.merge!(client_replacement)
# If there is only one driver, we can use generic keys without driver short name.
if one_driver
@replacement_map.merge!({ '@driver_short_name@' => driver.short })
@replacement_map.merge!(replacement)
end
endSigned-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Extensions will be used to run some commands independently of the driver. We need this to have generic logic and prevent config duplication in drivers for external software. For example, to enable hcktrace extra software. Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
Signed-off-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>
c702b41 to
46f5803
Compare
No description provided.