SED-4555 local-execution-of-ap-with-keyword-accessing-the-ap-content-is-not-supported-30#602
Conversation
… for step 29 (#598) * SED-4555 Local execution of AP with Keyword accessing the AP content is not supported * SED-4555 PR feedback and cleanup * SED-4555 ClassLoaderArchiver must exclude signature files * SED-4555 switching to sequential unzip * SED-4555 PR feedbacks * SED-4562 Support Junit AP tests with Keyword accessing the AP content for Step 29 * SED-4562 PR feedbacks * SED-4562 fixing cases where operation mode is null in execution context
Summary of ChangesHello, 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 new Highlights
Changelog
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the local execution of Automation Packages, particularly for JUnit runners, to better support keywords that need to access AP content. It introduces a new LOCAL_AUTOMATION_PACKAGE operation mode and a mechanism to create a temporary physical archive from the classpath. This allows local executions to behave more like remote ones. The isClasspathBased flag has been removed, and many parts of the code have been updated to use the new OperationMode.isLocal() helper.
My review has identified a critical issue in pom.xml where a temporary snapshot dependency must be removed before merging. I also found an opportunity to improve null handling in a new sample keyword by adhering to our non-null contract guidelines, and a minor issue with silent error handling during temporary file cleanup. The rest of the changes appear to be a solid and consistent implementation of the new local execution strategy.
| <!-- TODO local overwrite of exense-commons, delete before merge --> | ||
| <dependency> | ||
| <groupId>ch.exense.commons</groupId> | ||
| <artifactId>exense-basic-commons</artifactId> | ||
| <version>0.0.0-SED-4555-SNAPSHOT</version> | ||
| </dependency> |
There was a problem hiding this comment.
This temporary dependency and the associated TODO comment should be removed before this pull request is merged. Leaving snapshot dependencies, especially with local overwrites, can cause build instability and conflicts for other developers.
| output.startMeasure("retrieving AP content"); | ||
| File automationPackageFile = retrieveAndExtractAutomationPackage(); | ||
| output.stopMeasure(); | ||
| String[] list = Objects.requireNonNull(automationPackageFile.list()); |
There was a problem hiding this comment.
According to repository guidelines, we prefer enforcing non-null contracts for critical parameters by fixing callers, rather than adding defensive null checks. The retrieveAndExtractAutomationPackage() method should ideally not return null if an automation package is expected. Please investigate why retrieveAndExtractAutomationPackage() can return null and address the root cause, either by ensuring it never returns null under valid conditions or by clearly defining its nullability contract and handling it at the appropriate level, potentially upstream. If automationPackageFile.list() can return null, it suggests automationPackageFile might not be a valid directory, which also points to an issue in the package retrieval/extraction logic.
References
- Prefer enforcing non-null contracts for critical parameters (e.g., execution context, execution ID) by fixing callers, rather than adding defensive null checks.
There was a problem hiding this comment.
Does not make sense, the requireNonNull is applied to the results of File.list on which we have no control.
| public void close() { | ||
| if (temporaryFile != null && temporaryFile.exists()) { | ||
| temporaryFile.delete(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The close() method silently ignores the return value of temporaryFile.delete(). If the file deletion fails (e.g., due to permissions or the file being in use), it would be beneficial to log a warning. This would help in diagnosing issues with leftover temporary files.
step-core/src/main/java/step/automation/packages/JavaAutomationPackageArchive.java
Show resolved
Hide resolved
| import java.io.IOException; | ||
| import java.util.Map; | ||
|
|
||
| public class LocalFunctionHandler extends KeywordHandler { |
There was a problem hiding this comment.
I have the impression that this class isn't required anymore
2 participants
No description provided.